Skip to content

Conversation

@yashykt
Copy link
Contributor

@yashykt yashykt commented Apr 16, 2025

Addresses #3364 for my immediate needs

Changes

Added a PrintTo method for PointDataAttributes. (We could probably do this for all the other types, but for my immediate needs, this is enough.)

I'm not sure where the maintainers would prefer such functions to be placed. Open to suggestions.

@yashykt yashykt requested a review from a team as a code owner April 16, 2025 22:41
@netlify
Copy link

netlify bot commented Apr 16, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit d188585
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/680142a2c3e4a90008c2b922

@codecov
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.56%. Comparing base (f987c9c) to head (d188585).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3365   +/-   ##
=======================================
  Coverage   89.56%   89.56%           
=======================================
  Files         210      210           
  Lines        6502     6502           
=======================================
  Hits         5823     5823           
  Misses        679      679           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lalitb
Copy link
Member

lalitb commented Apr 16, 2025

@yashykt - Can you revisit this PR, it seems to add/update ~1000 files.

@ThomsonTan
Copy link
Contributor

Seems some format is applied by accident?

@lalitb
Copy link
Member

lalitb commented Apr 17, 2025

Seems some format is applied by accident?

Actually, complete python venv has been added to the PR 🤦

@yashykt
Copy link
Contributor Author

yashykt commented Apr 17, 2025

my bad, and I thought I was careful :D

@lalitb
Copy link
Member

lalitb commented Apr 17, 2025

@yashykt - I do see some concerns maintaining a test-only helper that isn’t covered by the spec in the repo, as we want to keep the public API minimal and clean. Wouldn’t it make more sense for users of the library to add their own PrintTo implementation in their test code, within the appropriate otel namespace, if they need custom GTest output?

@yashykt
Copy link
Contributor Author

yashykt commented Apr 17, 2025

Wouldn’t it make more sense for users of the library to add their own PrintTo implementation in their test code, within the appropriate otel namespace, if they need custom GTest output?

Adding methods to some other library's namespace is generally not recommended due to namespace pollution, conflicts, and general maintainability. But I guess I'll leave the decision to y'all. Do you feel comfortable with users adding their own methods to opentelemetry::sdk::metrics?

Alternative, we could create a separate test_util library?

@lalitb
Copy link
Member

lalitb commented Apr 17, 2025

Adding methods to some other library's namespace is generally not recommended due to namespace pollution, conflicts, and general maintainability. But I guess I'll leave the decision to y'all. Do you feel comfortable with users adding their own methods to opentelemetry::sdk::metrics?

Agree on the namespace pollution/conflicts concerns. I'm not a GTest expert, but I believe there should be alternatives to directly implementing PrintTo in our namespace. Users could employ custom formatters, wrappers, or mappers in their own test code instead.

I have some concerns about adding a custom formatter directly in the OpenTelemetry repository. If users start depending on such implementation, we'd be obligated to maintain it indefinitely to avoid breaking their tests. This creates an unnecessary maintenance burden that isn't part of our core functionality.

@marcalff
Copy link
Member

I have some concerns about adding a custom formatter directly in the OpenTelemetry repository. If users start depending on such implementation, we'd be obligated to maintain it indefinitely to avoid breaking their tests. This creates an unnecessary maintenance burden that isn't part of our core functionality.

I second that.

Ok to have helpers for testing, but this should be test code in a test library, not production code in the opentelemetry metrics library.

In other words, we do not want to:

  • maintain a non official format, or have people create dependencies on this from their application
  • bloat the production library with test code.

@yashykt
Copy link
Contributor Author

yashykt commented Apr 18, 2025

ack

@yashykt yashykt closed this Apr 18, 2025
@yashykt yashykt mentioned this pull request Apr 21, 2025
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.

4 participants