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

[extension/encoding/awscloudwatchmetricstreams_encoding] Support unmarshalling JSON-formatted CloudWatch Metric Streams #38419

Merged

Conversation

constanca-m
Copy link
Contributor

@constanca-m constanca-m commented Mar 6, 2025

Description

This PR implements the translation of JSON-formatted CloudWatch Metric Streams into OpenTelemetry metrics.

It receives a record, and splits that record by each new line. Each of these parts will correspond to a JSON CloudWatch Metric Stream. It then translates this metric into an OpenTelemetry metric. The obtained metrics from the record are returned.

It follows the same logic as https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/awsfirehosereceiver/internal/unmarshaler/cwmetricstream.

Link to tracking issue

Fixes #38407.

Testing

I have added unit tests to:

  • Check we can extract open telemetry metrics from JSON valid cloudwatch metric stream
  • Check there are no metrics extracted if the data does not have expected format (for example, no unit or no value)
  • Check we can obtain metrics from a record containing both valid and invalid cloudwatch metric stream.

You can easily read and study what we get to what we return in the testdata/json files of the extension.

Documentation

The coding comments and the README file should be enough for documentation.

Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks @constanca-m! Looking good, main concerns are to do with how the tests are implemented.

Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Comment on lines +89 to +92
// joinMetricsFromFile reads the metrics inside the files,
// and joins them in the format a record expects it to be:
// each metric is expected to be in 1 line, and every new
// line marks a new metric
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I like this approach.

@constanca-m
Copy link
Contributor Author

@open-telemetry/collector-contrib-approvers Can I have a review here?

@mx-psi
Copy link
Member

mx-psi commented Mar 10, 2025

@constanca-m Can you fix the CI issues?

@constanca-m
Copy link
Contributor Author

@mx-psi Took a few attempts, but the CI issues are all gone now.

@axw
Copy link
Contributor

axw commented Mar 11, 2025

@MovieStoreGuy can we merge this despite the build-examples failure? It's unrelated - see #38488. I'm looking into it, no clue what's going on yet...

@MovieStoreGuy MovieStoreGuy merged commit 4bb3985 into open-telemetry:main Mar 11, 2025
170 of 171 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 11, 2025
@constanca-m constanca-m deleted the add-json-cw-metric-stream branch March 11, 2025 07:09
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.

Support unmarshalling JSON-formatted CloudWatch Metric Streams
5 participants