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

Add new options to spanmetrics processor in static mode: #5407

Merged

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Oct 6, 2023

PR Description

Adding new config options to spanmetrics processor in static mode:

  • aggregation_temporality: configures whether to reset the metrics after flushing.
  • metrics_flush_interval: configures how often to flush generated metrics.

Which issue(s) this PR fixes

Fixes #4614

Notes to the Reviewer

This change is ok from a feature parity point of view because both of these config options are present in otelcol.connector.spanmetrics.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@ptodev ptodev requested review from a team and clayton-cornell as code owners October 6, 2023 16:38
@ptodev ptodev linked an issue Oct 6, 2023 that may be closed by this pull request
CHANGELOG.md Outdated Show resolved Hide resolved
@ptodev ptodev force-pushed the 4614-spanmetrics-wire-in-new-metrics_flush_interval-field branch from c408bee to a7e44f6 Compare October 11, 2023 16:58
@ptodev
Copy link
Contributor Author

ptodev commented Oct 11, 2023

@tpaschalis thank you for the review, and apologies for the silly errors! Btw I see that we seem to have a practice to list supported values like this:

[ backend: <string> | default = "stdout" | supported = "stdout", "logs_instance" ]

I didn't do this for aggregation_temporality because it'd make the line too long and it looks awkward. But let me know if you feel strongly about it and I'll add this in. There are also a few other places which don't use this syntax to list supported values, so we could add it there too.

@ptodev ptodev requested a review from tpaschalis October 11, 2023 17:01
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Doc input is minimal. OK as-is.

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Oct 11, 2023
Copy link
Member

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ptodev ptodev force-pushed the 4614-spanmetrics-wire-in-new-metrics_flush_interval-field branch from a7e44f6 to 4a50bcc Compare October 16, 2023 13:31
@ptodev ptodev force-pushed the 4614-spanmetrics-wire-in-new-metrics_flush_interval-field branch from dd7f5c3 to 8d63542 Compare October 19, 2023 08:54
@ptodev ptodev merged commit 1d06721 into main Oct 19, 2023
7 checks passed
@ptodev ptodev deleted the 4614-spanmetrics-wire-in-new-metrics_flush_interval-field branch October 19, 2023 09:29
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanmetrics: wire in new metrics_flush_interval field
3 participants