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

[nginx] Better process status output for good metric names #1053

Merged
merged 5 commits into from
Mar 15, 2018

Conversation

zippolyte
Copy link
Contributor

@zippolyte zippolyte commented Jan 25, 2018

What does this PR do?

With this PR, the output of the status page is better processed so that streams and slabs metrics are correctly named and tagged. Also update the metadata.csv file to add all the missing metrics.

Motivation

What inspired you to submit this pull request?

Testing Guidelines

An overview on testing
is available in our contribution guidelines.

Versioning

  • Bumped the check version in manifest.json
  • Bumped the check version in datadog_checks/{integration}/__init__.py
  • Updated CHANGELOG.md. Please use Unreleased as the date in the title
    for the new section.
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

This is a major version change and is breaking for orgs that use the badly named metrics in dashboards or monitor since the names will change with this PR and the old names won't be sent anymore. We need to notify them.

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Can you please explain further exactly what metrics are are being deprecated/removed? Are you saying metrics were previously being sent with tags inside them?

metric_name = '%s.%s' % (metric_base, key)
output.extend(cls._flatten_json(metric_name, parsed[key], tags))
output = []
output.extend(cls._flatten_json(metric_base, parsed, tags))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just put return cls._flatten_json(metric_base, parsed, tags).

if key in TAGGED_KEYS:
metric_name = '%s.%s' % (metric_base, TAGGED_KEYS[key])
for tag_val, data in val2.iteritems():
tag = '%s:%s' % (TAGGED_KEYS[key], tag_val)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say instead tags.append('%s:%s' % (TAGGED_KEYS[key], tag_val)).

Copy link
Contributor Author

@zippolyte zippolyte Mar 15, 2018

Choose a reason for hiding this comment

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

After seeing the tests fail miserably, I remembered why it was like this. I can't append to the tags list because I am in a for loop, and tags is a reference to the list. So I have to do like this to avoid having the tags of a metric being sent with the metrics of all the subsequent loops.

metric_name = '%s.%s' % (metric_base, TAGGED_KEYS[key])
for tag_val, data in val2.iteritems():
tag = '%s:%s' % (TAGGED_KEYS[key], tag_val)
output.extend(cls._flatten_json(metric_name, data, tags + [tag]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the above change, just pass tags.

@zippolyte
Copy link
Contributor Author

zippolyte commented Mar 15, 2018

The metrics that are deprecated are the metrics prefixed with nginx.slabs and nginx.stream. The payload returned by the NGINX API changed since the check was first written, and the added metrics were then incorrectly named. Indeed, some parts of the metric names should be sent as tags instead.

e.g. there was a metric for stream that was being named
nginx.stream.server_zones.<user defined name>.connections, where the user defined name is a name defined in the NGINX configuration of the user.
Thus resulting in several distinct metrics nginx.stream.server_zones.*.connections, not really usable in the interface of the app.
Instead, the user defined name is now sent as a tag, and the metric is named nginx.stream.server_zones.connections, and the user can use the tags to slice and dice in Datadog.

I hope I am being clear.

@ofek
Copy link
Contributor

ofek commented Mar 15, 2018

Thanks for explaining!

@zippolyte zippolyte merged commit 585bb96 into master Mar 15, 2018
@zippolyte zippolyte deleted the hippo/nginx_metrics branch March 15, 2018 15:47
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

2 participants