-
Notifications
You must be signed in to change notification settings - Fork 183
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
space_as parameter #7
Conversation
### space_as (string) (optional) | ||
|
||
The space (`' '`) in the name or value of fields will be replaced with the given string in output. |
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.
We should sync with @frankreno to see if this is acceptable. I feel name is fine, but changing value is bit weird.
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.
Okay... we can ask @frankreno for input here, but we don't have much choice, since AFAIK we don't support query any dimensions name/value includes space
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.
By looking into the Carbon2Parser
code in receiver, looks like it just simply not allow any whitespace in key or value and will throw exception. There is no way to escape whitespace also.
The metrics 2.0 spec doesn't give any escaping approach as well. It just said "tags are space separated. Note double space to separate tags from meta_tags" so I guess not very helpful either
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 guess for carbon2, it makes sense that no space allowed as it's ambiguous (you can't differentiate if it's a delimiter or part of value). But does Prometheus have the same constraint? Wonder if it's a common case, we will get spaces in key or value. Also we should be consistent with the way our backend parses Prometheus format, but not carbon2 format?
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.
Prometheus doesn't have such constraint so the Prometheus format parser don't need to handle whitespace case either (it using ,
as separator between tags and ""
enclosing value).
I guess this is an edge case but consider Prometheus/Kubernetes support customized tags it's a necessary to have a safe net anyway.
On the other hand, is that possible to query some dimension with name/value include whitespace? If we cannot do it anyway on UI, keep whitespace in our ingested metadata will only cause confusing. right?
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 like we retain white space in value in PrometheusParser
if the metrics is sent in Prometheus format. Could you double check that? That means, if we want to retain the format, we will have to convert time series to Prometheus format. I'm fine with merging now to unblock QE. But before deciding if we need send Prometheus format eventually, we'd like to understand:
- Confirm if our backend retains spaces for prometheus metrics.
- Understand where do those metrics with spaces key/value come from? If we see them often or just a few specific metrics having it?
- Understand if our UI support searching kay/value with spaces.
cc @frankreno
Add collectos for nytimes/spg-productservice
Since the space is not allowed in carbon V2 tags (name or value), add a parameter
space_as
to handle this case with replace all spaces in tags to a given string (by default use_
)