Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Labels from a path as oc-collector attributes #463

Merged
merged 15 commits into from
Apr 13, 2020
Merged

Add Labels from a path as oc-collector attributes #463

merged 15 commits into from
Apr 13, 2020

Conversation

Pothulapati
Copy link
Contributor

Fixes linkerd/linkerd2#4008

This PR makes the proxy read file present at /var/run/linkerd/podinfo/labels and convert it into a HashMap and use those labels as attributes for spans/traces.

linkerd/linkerd2#4199 updates the control-plane helm charts to mount pod labels as file to the proxy coontainer using downwardAPI

With these changes, Traces would have pod labels as attributes, Allowing us to directly link jager dashboard links from the Linkerd dashboard (like we do with Grafana), part of linkerd/linkerd2#4177 . This was not possible previously because we didn't have a way to know the distinguish traces based on workloadKind and workloadName

Screenshot_2020-03-25 Jaeger UI
image

P.S: This is my first take in Rust and the proxy, Sorry if there are any mistakes :)
Signed-off-by: Tarun Pothulapati tarunpothulapati@outlook.com

@Pothulapati
Read labels from the path and add them to the oc-collector attributes
4b6394c 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati Pothulapati changed the title Read labels from the path and add them to the oc-collector attributes Add Labels from a path as oc-collector attributes Mar 25, 2020
@Pothulapati
Rust formatting fix
bc52933 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
olix0r
olix0r reviewed Mar 25, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
olix0r
olix0r reviewed Mar 25, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
hawkw
hawkw reviewed Mar 25, 2020
View reviewed changes
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few minor nits & suggestions. In general, I agree with Oliver's comments on the label parsing code.

linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/oc_collector.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
hawkw
hawkw reviewed Mar 25, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
@Pothulapati
Make convert fn more safer by using and_then & filter_map
9063333 
Also, unit tests for convert fn

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
hawkw
hawkw reviewed Mar 25, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
@Pothulapati
unwrap before to avoid overhead of an option
8173378 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
empty values for tests
48bf815 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
move labels path as a var
575ae26 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
add more invalid label tests
2ebc45c 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
Copy link
Contributor Author

@hawkw @olix0r K8s has gurantees on what kinds of keys and values it allows as labels. It allows the value to be empty in which case, the pair is written to the file as new="", which we succesfully consider. and we do the same even when key is empty (though k8s dosen't allow them)

One concern I have is for future cases (i.e mesh expansion, etc), For Labels without value, Users have to add =, as we ignore labels without = even though value is empty, Should we allow that?

@olix0r
Copy link
Member

olix0r commented Mar 26, 2020

@Pothulapati

For Labels without value, Users have to add =, as we ignore labels without = even though value is empty, Should we allow that?

Can you elaborate on this? Can you give an example of how a user would interact with this? I would imagine that users never have to interact with any of this stuff outside of the normal k8s tooling?

@Pothulapati
Copy link
Contributor Author

@olix0r Yep, no direct interaction as you mentioned but my concern was for future cases outside k8s (mesh expansion? ), where users would have to add the labels manually into a file, and even for labels without values, = has to be added.

@olix0r
Copy link
Member

olix0r commented Mar 26, 2020

where users would have to add the labels manually into a file

I don't think we'd ever do this, fwiw.

@Pothulapati
use unwrap_or_default to not panic when labels is Err
b59cd73 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
hawkw
hawkw approved these changes Mar 27, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
olix0r
olix0r reviewed Mar 27, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
@Pothulapati
Add warnings for each case by using match and separate init into a fn
eb64caa 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati Pothulapati mentioned this pull request Apr 2, 2020
Merged
@Pothulapati
Update labels to also include namespace
d8e6275 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
Copy link
Contributor Author

Updated the proxy code to also include namespace label. Especially useful for jaeger integration into the Linkerd dashboard.
Screenshot_2020-04-06 Jaeger UI(1)
Screenshot_2020-04-06 Jaeger UI

olix0r
olix0r requested changes Apr 8, 2020
View reviewed changes
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to merge this, we at least need to back out the current approach to namespaces. If namespaces are required, we need to figure out an alternate approach.

linkerd/app/src/oc_collector.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
hawkw
hawkw reviewed Apr 8, 2020
View reviewed changes
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I agree with @olix0r's review. I also left some notes on idiomatic Rust that I hope you find helpful.

linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
@Pothulapati
Revert "Update labels to also include namespace"
1cfb50d 
This reverts commit d8e6275.

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
separate oc_trace_labels fn, by dividing error handling
f10d287 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
remove warning when env is not present
3dfd597 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@Pothulapati
remove env warnings
a165bf8 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
olix0r
olix0r reviewed Apr 10, 2020
View reviewed changes
linkerd/app/src/env.rs Outdated Show resolved Hide resolved
@Pothulapati
update naming terminology from labels to attributes
89c0c65 
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
olix0r
olix0r approved these changes Apr 13, 2020
View reviewed changes
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Pothulapati !

@olix0r olix0r merged commit 270d343 into linkerd:master Apr 13, 2020
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Apr 15, 2020
@olix0r
proxy: v2.92.0
0f8a790 
This release includes a new protocol detection timeout, which prevents
clients from consuming resources indefinitely when they do not send any
data.

Additionally: the proxy's admin endpoint now supports a `/live` endpoint
for liveness checks, and a feature has been added to enrich tracing
metadata from a file of label/values.

---

* Add Labels from a path as oc-collector attributes (linkerd/linkerd2-proxy#463)
* Add liveness endpoint to admin server (linkerd/linkerd2-proxy#470)
* docker: Use buildkit for caching (linkerd/linkerd2-proxy#472)
* Makefile: Use STRIP variable with strip as default (linkerd/linkerd2-proxy#475)
* Add checksec to the release process (linkerd/linkerd2-proxy#476)
* Time out protocol detect futures (linkerd/linkerd2-proxy#464)
* Ensure that checksec is executable (linkerd/linkerd2-proxy#477)
* Fix the checksec URL (linkerd/linkerd2-proxy#478)
* Undo hardcoded release version (linkerd/linkerd2-proxy#479)
@olix0r olix0r mentioned this pull request Apr 15, 2020
Merged
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Apr 16, 2020
@olix0r
proxy: v2.92.0 (#4264)
b358763 
This release includes a new protocol detection timeout, which prevents
clients from consuming resources indefinitely when they do not send any
data.

Additionally: the proxy's admin endpoint now supports a `/live` endpoint
for liveness checks, and a feature has been added to enrich tracing
metadata from a file of label/values.

---

* Add Labels from a path as oc-collector attributes (linkerd/linkerd2-proxy#463)
* Add liveness endpoint to admin server (linkerd/linkerd2-proxy#470)
* docker: Use buildkit for caching (linkerd/linkerd2-proxy#472)
* Makefile: Use STRIP variable with strip as default (linkerd/linkerd2-proxy#475)
* Add checksec to the release process (linkerd/linkerd2-proxy#476)
* Time out protocol detect futures (linkerd/linkerd2-proxy#464)
* Ensure that checksec is executable (linkerd/linkerd2-proxy#477)
* Fix the checksec URL (linkerd/linkerd2-proxy#478)
* Undo hardcoded release version (linkerd/linkerd2-proxy#479)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@olix0r olix0r olix0r approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Owner Metadata for Spans.
3 participants
@Pothulapati @olix0r @hawkw