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

Make ignore_metrics support * wildcard for OpenMetrics #5759

Merged
merged 1 commit into from
Feb 17, 2020

Conversation

ahmed-mez
Copy link
Contributor

@ahmed-mez ahmed-mez commented Feb 17, 2020

What does this PR do?

  • Metrics can be ignored if they match afnmatchcase pattern
  • Document the ignore_metrics openmetrics check parameter

Motivation

More flexible config to ignore metrics

Additional Notes

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

Comment on lines 149 to 157
config['_ignored_metrics'] = {}
config['_ignored_patterns'] = {}

# Separate ignored metric names and ignored patterns in different maps for faster lookup later
for metric in config['ignore_metrics']:
if '*' in metric:
config['_ignored_patterns'][metric] = True
else:
config['_ignored_metrics'][metric] = True
Copy link
Member

Choose a reason for hiding this comment

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

Can’t we use sets rather than dictionnary ?
It seems that the only value that we use is True and we only check for element existence.

Suggested change
config['_ignored_metrics'] = {}
config['_ignored_patterns'] = {}
# Separate ignored metric names and ignored patterns in different maps for faster lookup later
for metric in config['ignore_metrics']:
if '*' in metric:
config['_ignored_patterns'][metric] = True
else:
config['_ignored_metrics'][metric] = True
config['_ignored_metrics'] = set()
config['_ignored_patterns'] = set()
# Separate ignored metric names and ignored patterns in different maps for faster lookup later
for metric in config['ignore_metrics']:
if '*' in metric:
config['_ignored_patterns'].add(metric)
else:
config['_ignored_metrics'].add(metric)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed thanks!

@ahmed-mez ahmed-mez force-pushed the ahmed-mez/option-to-exclude-metrics branch from fea930c to 005b04e Compare February 17, 2020 13:23
self._send_telemetry_counter(
self.TELEMETRY_COUNTER_METRICS_IGNORE_COUNT, len(metric.samples), scraper_config
)
return # Ignore the metric

for ignore_pattern in scraper_config['_ignored_patterns']:
if fnmatchcase(metric.name, ignore_pattern):
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause a large slow down. No way around it I guess, just saying...

Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth adding a if config['ignore_metrics'] or the for on the empty set if fast enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is a small benchmark - it's almost the same

>>> timeit('for x in s:    print("found")', setup='s = set(); x = "randomstring"',number=100000)
0.004889011383056641
>>>
>>> timeit('if map:    print("found")', setup='map = dict()', number=100000)
0.003404855728149414

Copy link
Contributor

Choose a reason for hiding this comment

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

in absolute it's fast yes, but looks like we could use the 30% speedup 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is a follow-up PR #5764

@ofek ofek changed the title Make ignore_metrics support "*" wildcard Make ignore_metrics support * wildcard for OpenMetrics Feb 17, 2020
@ahmed-mez ahmed-mez merged commit f72425b into master Feb 17, 2020
@ahmed-mez ahmed-mez deleted the ahmed-mez/option-to-exclude-metrics branch February 17, 2020 17:16
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.

4 participants