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

[receiver/windowsperfcounters] Clarify current treatment of the _Total instance #33692

Conversation

pjanotti
Copy link
Contributor

Description:
Clarifies the current treatment of the _Total instances.

Link to tracking Issue:
Fix #29054

Testing:
Manually validated the configuration added to the README.md

Documentation:
Described the behavior of the _Total instance as currently implemented.

@pjanotti
Copy link
Contributor Author

cc @alxbl @crobert-1

@pjanotti pjanotti changed the title Clarify current treatment of the _Total instance [receiver/windowsperfcounters] Clarify current treatment of the _Total instance Jun 21, 2024
metric: processor.time.total
```

> [!WARNING]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually gives an idea to how better handle counters that don't use _Total for their aggregation. We can have a configuration called something like aggregation_name, that by default is _Total, if someone is using a counter that uses a different name for the aggregation instance we let them specify so the problem of double counting can be easily solved.

Copy link
Member

Choose a reason for hiding this comment

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

The aggregation_name would be used to decide which instance to drop from a wildcard collection? That makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so it keeps the current default behavior, but, if some user wants to drop the _Global_ they add that to the configuration.

@crobert-1 crobert-1 added Skip Changelog PRs that do not require a CHANGELOG.md entry documentation Improvements or additions to documentation labels Jun 21, 2024
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Thanks for adding more documentation here @pjanotti! Just had some formatting comments/questions.

receiver/windowsperfcountersreceiver/README.md Outdated Show resolved Hide resolved
receiver/windowsperfcountersreceiver/README.md Outdated Show resolved Hide resolved
receiver/windowsperfcountersreceiver/README.md Outdated Show resolved Hide resolved
receiver/windowsperfcountersreceiver/README.md Outdated Show resolved Hide resolved
pjanotti and others added 4 commits June 21, 2024 11:14
Co-authored-by: Curtis Robert <crobert@splunk.com>
…n' of github.com:pjanotti/opentelemetry-service-contrib into windows-perf-counters-aggregation-instance-clarification
@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Jun 21, 2024
@dmitryax dmitryax merged commit baebad7 into open-telemetry:main Jun 25, 2024
138 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ready to merge Code review completed; ready to merge by maintainers receiver/windowsperfcounters Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[receiver/windowsperfcounters] Instance wildcard doesn't capture _Total instance
5 participants