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

space_as parameter #7

Merged
merged 2 commits into from
Apr 11, 2019
Merged

space_as parameter #7

merged 2 commits into from
Apr 11, 2019

Conversation

bin3377
Copy link

@bin3377 bin3377 commented Apr 8, 2019

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 _)

### space_as (string) (optional)

The space (`' '`) in the name or value of fields will be replaced with the given string in output.
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Author

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

Copy link
Contributor

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?

Copy link
Author

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?

Copy link
Contributor

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:

  1. Confirm if our backend retains spaces for prometheus metrics.
  2. Understand where do those metrics with spaces key/value come from? If we see them often or just a few specific metrics having it?
  3. Understand if our UI support searching kay/value with spaces.

cc @frankreno

@lei-sumo lei-sumo requested a review from frankreno April 9, 2019 17:07
@bin3377 bin3377 merged commit 46d16b7 into master Apr 11, 2019
@bin3377 bin3377 deleted the byi-fix-space branch April 11, 2019 17:01
psaia pushed a commit to psaia/sumologic-kubernetes-collection that referenced this pull request May 25, 2021
Add collectos for nytimes/spg-productservice
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.

None yet

4 participants