-
Notifications
You must be signed in to change notification settings - Fork 562
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
Allow custom labels to be added to net/http metrics #306
Conversation
There was a problem hiding this 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.
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 |
* Add mutex around access to labels * Add Labeler use to handler example test
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. |
I reviewed this PR and thought about the underlying request. 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. |
…ontrib into http_metric_labeler
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. |
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
Fixes #195