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

Fix namespace valid chars matching regex #6352

Merged
merged 5 commits into from
Apr 15, 2020

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Apr 15, 2020

cc @ian28223 @therve

What does this PR do?

Fix namespace valid chars matching regex

Namespace might contains chars like -: https://www.aerospike.com/docs/guide/limitations.html

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@@ -274,10 +274,16 @@ def collect_latency(self, namespaces):
continue

# match only works at the beginning
ns_metric_name_match = re.match(r'{(\w+)}-(\w+):', line)
# ':' or ';' are not allowed in namespace-name: https://www.aerospike.com/docs/guide/limitations.html
ns_metric_name_match = re.match(r'{([^\}:;]+)}-(\w+):', line)
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. Could you take the opportunity to compile the regex in init?

Copy link
Member Author

Choose a reason for hiding this comment

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

@therve Regexes are cached by python, not sure it worth moving them away from where they are used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really in a reliable fashion though. If you have another integration in the same agent uses lots of regexp, then your cache is gone.

I think moving them a bit but giving them a readable name makes sense.

Copy link
Member Author

@AlexandreYang AlexandreYang Apr 15, 2020

Choose a reason for hiding this comment

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

§ python2 -c 'import re; print(re._MAXCACHE)'
100
§ python3 -c 'import re; print(re._MAXCACHE)'
512

The regex cache is quite small. Yeah, probably better to not rely on the cache.

If you don't mind, I will do a refactoring to pre compile all the regexes in a separate PR. We might want to cherry pick this PR (probably not, just in case).

@AlexandreYang AlexandreYang requested review from therve April 15, 2020 14:30
@AlexandreYang AlexandreYang merged commit dade4c6 into master Apr 15, 2020
@AlexandreYang AlexandreYang deleted the alex/aerospike_fix_namespace_valid_chars branch April 15, 2020 14:46
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.

2 participants