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.

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

Allow custom labels to be added to net/http metrics #306

Merged
merged 7 commits into from
Sep 9, 2020

Conversation

Aneurysm9
Copy link
Member

Fixes #195

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good. Aside from the question on needing a mutex, nothing is blocking.

instrumentation/net/http/labeler.go Show resolved Hide resolved
instrumentation/net/http/labeler.go Outdated Show resolved Hide resolved
instrumentation/net/http/labeler.go Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Aug 27, 2020

I wonder if this functionality ought to be achieved using Correlation Context. The OpenCensus system had a TTL=0 concept that meant the correlations would only survive within a single process. Then we wouldn't need a new API here, I think.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 27, 2020

I wonder if this functionality ought to be achieved using Correlation Context. The OpenCensus system had a TTL=0 concept that meant the correlations would only survive within a single process. Then we wouldn't need a new API here, I think.

That's a really interesting idea. It could make this solution more universal for all instrumentation.

@Aneurysm9
Copy link
Member Author

I wonder if this functionality ought to be achieved using Correlation Context. The OpenCensus system had a TTL=0 concept that meant the correlations would only survive within a single process. Then we wouldn't need a new API here, I think.

That's a really interesting idea. It could make this solution more universal for all instrumentation.

I wonder how this would work to get data from the handler up to the instrumentation. It seems that context flow is one-way. Or are you proposing that the instrumentation add the Labeler to the Correlation Context rather than directly to the context.Context?

* Add mutex around access to labels
* Add Labeler use to handler example test
@MrAlias
Copy link
Contributor

MrAlias commented Aug 31, 2020

I wonder how this would work to get data from the handler up to the instrumentation. It seems that context flow is one-way. Or are you proposing that the instrumentation add the Labeler to the Correlation Context rather than directly to the context.Context?

Good point. I think I was envisioning the use to add context prior to it getting to the middleware of the handler, but I don't think that is going to be easily achieved nor desirable to a user. It seems like they would rather an explicit solution like what you have here.

@jmacd
Copy link
Contributor

jmacd commented Sep 1, 2020

I reviewed this PR and thought about the underlying request.
This feature offers an equivalent to the act of calling Span.SetAttribute() during a request, with the intent of adding labels to metrics associated with the effective Span. This is one reason people like to (at least) think about automatically converting spans to metrics--because we already have APIs to modify the current Span, therefore we wouldn't need new APIs to support adding labels to correlated metrics mid-span. My proposal about Resource scope was intended to address a similar problem: open-telemetry/oteps#78. That proposal stops short of adding a way to modify a current scope (equivalent to SetAttribute), it only gave us a way to create new scopes, but the same idea could be extended to support mutable scopes.

The use of Context in this PR, at a first glance, leads to confusion. We're only using context to pass a variable from the server to the handler. We would not expect the same labeler to be used in a child, that's why I prefer to think of this as "modify the current metric scope".

After studying this, Correlation Context is not a good way to propagate these labels--it would lead to an awkward API. Short of something radical like OTEP issue 78 (Resource scope), I don't see a way to avoid an interface like Labeler in this PR.

instrumentation/net/http/handler.go Outdated Show resolved Hide resolved
@Aneurysm9 Aneurysm9 requested a review from XSAM as a code owner September 8, 2020 22:45
@Aneurysm9 Aneurysm9 merged commit c55a925 into open-telemetry:master Sep 9, 2020
@derekperkins
Copy link

That's awesome, thanks for pushing that through! My only question on the PR is whether or not the labeler should be pulled out so that it can be shared between other implementations like grpc.

plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
Change all occurrences of value to pointer receivers
Add meta sys files to .gitignore
Code cleanup e.g.
- Don't capitalize error statements
- Fix ignored errors
- Fix ambiguous variable naming
- Remove unnecessary type casting
- Use named params

Fix #306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grpc/http: support custom KeyValues when reporting metrics
4 participants