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

remove leading ~ from name when indexing metric names #2458

Merged
merged 7 commits into from
Jan 19, 2020
Merged

Conversation

replay
Copy link
Collaborator

@replay replay commented Jun 26, 2019

No description provided.

replay added a commit to replay/carbon that referenced this pull request Jun 27, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@replay replay force-pushed the docs_update branch 2 times, most recently from f36cfc7 to 8b12425 Compare June 27, 2019 17:21
@replay replay changed the title remove ~ from name when indexing metric names remove leading ~ from name when indexing metric names Jun 27, 2019
@deniszh deniszh requested a review from DanCech August 14, 2019 10:10
Copy link
Member

@DanCech DanCech left a comment

Choose a reason for hiding this comment

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

This doesn't match the actual implementation in the TaggedSeries class, which will reject tagged series with ~ in the value.

replay added a commit to replay/carbon that referenced this pull request Aug 16, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@DanCech
Copy link
Member

DanCech commented Aug 16, 2019

We want to keep TaggedSeries the same between graphite-web and carbon (ideally we'd break it out into a separate graphite-util package but if we're going to move toward a go implementation of carbon it doesn't make sense to put in the work required to do that vs just keeping the class in sync across the repos). If you can sync them up that would be great, I'd also suggest that the sanitize function should use https://docs.python.org/2/library/string.html#string.lstrip vs replace.

@codecov-io
Copy link

codecov-io commented Aug 16, 2019

Codecov Report

Merging #2458 into master will decrease coverage by 0.91%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2458      +/-   ##
==========================================
- Coverage   80.05%   79.13%   -0.92%     
==========================================
  Files          85       85              
  Lines        8948     9150     +202     
  Branches     1914     1948      +34     
==========================================
+ Hits         7163     7241      +78     
- Misses       1511     1630     +119     
- Partials      274      279       +5
Impacted Files Coverage Δ
webapp/graphite/tags/redis.py 90.74% <50%> (-1.09%) ⬇️
webapp/graphite/tags/utils.py 79.74% <50%> (+0.33%) ⬆️
webapp/graphite/tags/localdatabase.py 93.66% <50%> (-0.71%) ⬇️
webapp/graphite/tags/base.py 91.79% <50%> (-2.66%) ⬇️
webapp/graphite/tags/http.py 94.8% <50%> (-5.2%) ⬇️
webapp/graphite/middleware.py 77.77% <0%> (-22.23%) ⬇️
webapp/graphite/render/evaluator.py 90.34% <0%> (-9.66%) ⬇️
webapp/graphite/url_shortener/views.py 42.85% <0%> (-9.53%) ⬇️
webapp/graphite/functions/params.py 90.54% <0%> (-9.46%) ⬇️
webapp/graphite/util.py 81.66% <0%> (-9.17%) ⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a103fa...9f67047. Read the comment docs.

@replay
Copy link
Collaborator Author

replay commented Aug 16, 2019

I'd also suggest that the sanitize function should use https://docs.python.org/2/library/string.html#string.lstrip vs replace

I'm not sure which sanitize function you mean, the one in the carbon project, right? If so, then I've already updated that to use .lstrip in https://github.com/graphite-project/carbon/pull/858/files
Or do you think that graphite should also sanitize the values when it receives a request to tag a series? So far graphite is only validating, but not sanitizing. Carbon is doing the sanitizing.

@replay
Copy link
Collaborator Author

replay commented Aug 16, 2019

I didn't realize that the TaggedSeries class in carbon and graphite is pretty much the same, i'll sync the carbon one to graphite then

@replay
Copy link
Collaborator Author

replay commented Aug 20, 2019

I've pushed a few more tests, so the test coverage report is happy

@replay
Copy link
Collaborator Author

replay commented Aug 27, 2019

Rebased onto the latest master

Co-Authored-By: Dan Cech <dan@aussiedan.com>
@DanCech DanCech requested a review from deniszh December 17, 2019 18:13
@deniszh deniszh merged commit 1527eb4 into master Jan 19, 2020
@deniszh deniszh deleted the docs_update branch January 19, 2020 20:25
deniszh pushed a commit to graphite-project/carbon that referenced this pull request Jan 19, 2020
* sanitize names when using them as tag value

related: graphite-project/graphite-web#2458

* implement updated rules

* copy tag and value validation from graphite

* better error messages

* add tests for tag validation

* Better exception messages

Co-Authored-By: Dan Cech <dan@aussiedan.com>

Co-authored-by: Dan Cech <dan@aussiedan.com>
pull bot pushed a commit to NeatNerdPrime/graphite-web that referenced this pull request Jan 19, 2020
…ct#2458)

* remove ~ from name when indexing metric names

* better wording

* update tag validation so it enforces the new rules as documented

* add sanitize for metric name and handle exceptions

* better error messages

* adding tests for tag validation

* Better exception messages

Co-Authored-By: Dan Cech <dan@aussiedan.com>

Co-authored-by: Dan Cech <dan@aussiedan.com>
deniszh pushed a commit to deniszh/carbon that referenced this pull request Jan 20, 2020
* sanitize names when using them as tag value

related: graphite-project/graphite-web#2458

* implement updated rules

* copy tag and value validation from graphite

* better error messages

* add tests for tag validation

* Better exception messages

Co-Authored-By: Dan Cech <dan@aussiedan.com>

Co-authored-by: Dan Cech <dan@aussiedan.com>
deniszh pushed a commit to deniszh/carbon that referenced this pull request Jan 20, 2020
* sanitize names when using them as tag value

related: graphite-project/graphite-web#2458

* implement updated rules

* copy tag and value validation from graphite

* better error messages

* add tests for tag validation

* Better exception messages

Co-Authored-By: Dan Cech <dan@aussiedan.com>

Co-authored-by: Dan Cech <dan@aussiedan.com>
deniszh pushed a commit to deniszh/carbon that referenced this pull request Feb 21, 2020
* sanitize names when using them as tag value

related: graphite-project/graphite-web#2458

* implement updated rules

* copy tag and value validation from graphite

* better error messages

* add tests for tag validation

* Better exception messages

Co-Authored-By: Dan Cech <dan@aussiedan.com>

Co-authored-by: Dan Cech <dan@aussiedan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants