Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Restoring metrics in requests #1110
Restoring metrics in requests #1110
Changes from 7 commits
784bb02
feb34d8
0d58aaa
cc33431
c5224fc
f39393d
f497101
4cca547
7f843cb
b872ada
5c8cacb
68e34ee
80bdff2
7db639b
89d8f6e
b273409
52de6c3
661843b
8a0e994
077f4be
7f4a49b
32dab15
e57651b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this
url
parametrized or plain raw url? Otherwise, this can lead very to high cardinality.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.
This is the same
url
we add in span attributes. I think its not parameterised. (i.e. when added in metric it would look something like thishttp.url: STRING(https://www.example.com/123/?key1=value1
)I cannot think of a way to find encoded parameters from url to remove them. (e.g. in the above example remove
123
from path).Open for suggestions :)
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.
That's not a problem with tracing but here each url creates a new time series which should be avoided. If there is no way to get the parametrized url I would suggest remove this tag entirely.
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.
Removed url tag. But there could be 2 different urls in a single time series (as there might be no differentiating tag). I am not sure if this is okay. Any suggestions?
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.
I imagine that the call to
urlparse
is what can cause theValueError
exception to be raised. If that happens, does it make sense to continue the instrumentation with ametrics_labels
dictionary that will not have"http.host"
and"http.scheme"
keys?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.
In traces we add the url even if the url is invalid (e.g. url =
random123
is also added as span attribute). So, I added it here for consistency.I think its okay to go without
http.host
andhttp.scheme
keys ashttp.url
attribute alone is one of the recommended set of attributes for client metrics (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives)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.
Are we missing a few?
net.*
,http.target
?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.
Added
net.*
attributes.I have not added
http.target
as the target is not parameterised and adding raw target can lead to high cardinality.Its similar for
http.url
as discussed here