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/dockerstats] Combine and simplify multiple blockio metrics by making "operation" an attribute #13445

Merged

Conversation

jamesmoessis
Copy link
Contributor

@jamesmoessis jamesmoessis commented Aug 22, 2022

Description:

Instead of having several blockio metrics with the operation appended on the name as .read, .write, .sync, .async, .total, .discard we pull the operation out into an attribute. This simplifies the code a lot:

  1. Much less generated code, since that scales with amount of metric names.
  2. Simplifies the recording logic, as we don't have to map the operation from the stat to a recording function, we put it in an attribute instead.
  3. It also makes sense from an otel/metrics perspective, as it more closely aligns with the approach of the semconv (tending towards using attributes instead of additional metric names where possible).

This is a breaking change for anyone using the featuregated v2 implementation, which is not enabled by default yet. To my knowledge, no one is using the new v2 implementation yet.

Link to tracking Issue: #9794

Testing:

The tests logic remain the same, but the expected metrics are updates to align with the changes.

Documentation:

Documentation automatically updated by go generate.

@jamesmoessis
Copy link
Contributor Author

@rmfitzpatrick @MovieStoreGuy FYI + please review if you have time 😄

Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

This change makes sense to me.

@jamesmoessis
Copy link
Contributor Author

Hi @rmfitzpatrick @djaglowski just bumping this PR

@rmfitzpatrick
Copy link
Contributor

These changes seem good to me, and I imagine similar updates can be made to the container.network.io.usage metrics w/ direction and ~state attributes, though that may be related to the outcome of open-telemetry/opentelemetry-specification#2726.

@jamesmoessis
Copy link
Contributor Author

@jpkrohling @djaglowski I have codeowner approval and one other approval. Can I get this reviewed/merged please 😃

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

approving by proxy

@jpkrohling jpkrohling merged commit ebc8ea2 into open-telemetry:main Sep 6, 2022
@jamesmoessis jamesmoessis deleted the combine-blockio-metrics branch September 6, 2022 22:42
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.

5 participants