Add Labels from a path as oc-collector attributes by Pothulapati · Pull Request #463 · linkerd/linkerd2-proxy

Pothulapati · 2020-03-25T09:28:39Z

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

P.S: This is my first take in Rust and the proxy, Sorry if there are any mistakes :)
Signed-off-by: Tarun Pothulapati [email protected]

Signed-off-by: Tarun Pothulapati <[email protected]>

linkerd/app/src/env.rs

hawkw

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

linkerd/app/src/oc_collector.rs

linkerd/app/src/env.rs

Also, unit tests for convert fn Signed-off-by: Tarun Pothulapati <[email protected]>

linkerd/app/src/env.rs

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2020-03-26T18:27:21Z

@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 · 2020-03-26T18:43:19Z

@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 · 2020-03-26T18:48:43Z

@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 · 2020-03-26T18:49:42Z

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

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

Signed-off-by: Tarun Pothulapati <[email protected]>

linkerd/app/src/env.rs

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati · 2020-04-06T10:50:20Z

Updated the proxy code to also include namespace label. Especially useful for jaeger integration into the Linkerd dashboard.

olix0r

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

linkerd/app/src/env.rs

hawkw

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

This reverts commit d8e6275. Signed-off-by: Tarun Pothulapati <[email protected]>

Signed-off-by: Tarun Pothulapati <[email protected]>

linkerd/app/src/env.rs

Signed-off-by: Tarun Pothulapati <[email protected]>

olix0r

Thanks, @Pothulapati !

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)

Read labels from the path and add them to the oc-collector attributes

4b6394c

Signed-off-by: Tarun Pothulapati <[email protected]>

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

Rust formatting fix

bc52933

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the trace-names branch from 3660139 to bc52933 Compare March 25, 2020 13:48

olix0r reviewed Mar 25, 2020

View reviewed changes

linkerd/app/src/env.rs Outdated Show resolved Hide resolved

olix0r reviewed Mar 25, 2020

View reviewed changes

linkerd/app/src/env.rs Outdated Show resolved Hide resolved

hawkw reviewed Mar 25, 2020

View reviewed changes

linkerd/app/src/env.rs Outdated Show resolved Hide resolved

Make convert fn more safer by using and_then & filter_map

9063333

Also, unit tests for convert fn Signed-off-by: Tarun Pothulapati <[email protected]>

hawkw reviewed Mar 25, 2020

View reviewed changes

linkerd/app/src/env.rs Outdated Show resolved Hide resolved

Pothulapati added 4 commits March 26, 2020 00:55

unwrap before to avoid overhead of an option

8173378

Signed-off-by: Tarun Pothulapati <[email protected]>

empty values for tests

48bf815

Signed-off-by: Tarun Pothulapati <[email protected]>

move labels path as a var

575ae26

Signed-off-by: Tarun Pothulapati <[email protected]>

add more invalid label tests

2ebc45c

Signed-off-by: Tarun Pothulapati <[email protected]>

use unwrap_or_default to not panic when labels is Err

b59cd73

Signed-off-by: Tarun Pothulapati <[email protected]>

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 reviewed Mar 27, 2020

View reviewed changes

linkerd/app/src/env.rs Outdated Show resolved Hide resolved

Add warnings for each case by using match and separate init into a fn

eb64caa

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati mentioned this pull request Apr 2, 2020

charts: Using downwardAPI to mount labels to the proxy container linkerd/linkerd2#4199

Merged

Update labels to also include namespace

d8e6275

Signed-off-by: Tarun Pothulapati <[email protected]>

olix0r requested changes Apr 8, 2020

View reviewed changes

hawkw reviewed Apr 8, 2020

View reviewed changes

Pothulapati force-pushed the trace-names branch from 3489746 to 6fd5a40 Compare April 9, 2020 07:45

Pothulapati added 2 commits April 9, 2020 14:20

Revert "Update labels to also include namespace"

1cfb50d

This reverts commit d8e6275. Signed-off-by: Tarun Pothulapati <[email protected]>

separate oc_trace_labels fn, by dividing error handling

f10d287

Signed-off-by: Tarun Pothulapati <[email protected]>

Pothulapati force-pushed the trace-names branch from 6fd5a40 to f10d287 Compare April 9, 2020 08:51

Pothulapati added 2 commits April 9, 2020 20:32

remove warning when env is not present

3dfd597

Signed-off-by: Tarun Pothulapati <[email protected]>

remove env warnings

a165bf8

Signed-off-by: Tarun Pothulapati <[email protected]>

olix0r reviewed Apr 10, 2020

View reviewed changes

linkerd/app/src/env.rs Outdated Show resolved Hide resolved

update naming terminology from labels to attributes

89c0c65

Signed-off-by: Tarun Pothulapati <[email protected]>

olix0r approved these changes Apr 13, 2020

View reviewed changes

olix0r merged commit 270d343 into linkerd:master Apr 13, 2020

olix0r mentioned this pull request Apr 15, 2020

proxy: v2.92.0 linkerd/linkerd2#4264

Merged

Conversation

Pothulapati commented Mar 25, 2020

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pothulapati commented Mar 26, 2020

Uh oh!

olix0r commented Mar 26, 2020

Uh oh!

Pothulapati commented Mar 26, 2020

Uh oh!

olix0r commented Mar 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pothulapati commented Apr 6, 2020

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants