Skip to content

[cmd/telemetrygen] Export telemetrygen functions for testing #37044

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

Merged

Conversation

Erog38
Copy link
Contributor

@Erog38 Erog38 commented Jan 6, 2025

Description

This PR addresses #36984 in order to open up telemetrygen to be used by golang tests and generate varying amounts of telemetry in code to support use cases which don't rely on external tools.

The following major changes were made to clear the footprint for how this API can be used:

  1. Moved metrics, traces, and logs under pkg instead of internal.
  2. Unexported the run function for each test suite so the entrypoint is only the Start function
  3. Moved config defaults from pflags into a SetDefaults function which is exported alongside NewConfig
    for changed packages in order for users to get the same sane defaults as if run from the command line.
  4. Adjusted the pflags to use the existing default values rather than duplicate their own removing default config duplication
  5. Added E2E tests for Metrics and Logs and adjusted Traces to use the new API so it stays consistent.
  6. Adjusted log and metric exporter instantiation slightly due to a race condition with an err variable which was only caught by the new E2E tests.

Testing

Added to the E2E tests to ensure consistent API specs.

@Erog38 Erog38 requested review from mx-psi, codeboten and a team as code owners January 6, 2025 23:45
@github-actions github-actions bot added the cmd/telemetrygen telemetrygen command label Jan 6, 2025
@Erog38 Erog38 changed the title feat: Export telemetrygen functions for testing [cmd/telemetrygen] Export telemetrygen functions for testing Jan 6, 2025
@Erog38
Copy link
Contributor Author

Erog38 commented Jan 6, 2025

This goes hand in hand with #37003 in order to help own and maintain this API. I read about chloggen, but haven't used it before so I copied the template and made a version manually. I hope I did that properly.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

Attention: Patch coverage is 15.17857% with 190 lines in your changes missing coverage. Please review.

Project coverage is 79.57%. Comparing base (1629eda) to head (a719b82).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
cmd/telemetrygen/pkg/logs/logs.go 8.88% 39 Missing and 2 partials ⚠️
cmd/telemetrygen/pkg/metrics/metrics.go 8.88% 39 Missing and 2 partials ⚠️
cmd/telemetrygen/internal/common/config.go 0.00% 29 Missing ⚠️
cmd/telemetrygen/pkg/traces/config.go 15.15% 28 Missing ⚠️
cmd/telemetrygen/pkg/logs/config.go 35.13% 24 Missing ⚠️
cmd/telemetrygen/pkg/metrics/config.go 0.00% 18 Missing ⚠️
cmd/telemetrygen/pkg/metrics/metrics_types.go 0.00% 5 Missing ⚠️
cmd/telemetrygen/pkg/traces/traces.go 33.33% 2 Missing ⚠️
cmd/telemetrygen/config.go 75.00% 1 Missing ⚠️
cmd/telemetrygen/pkg/metrics/worker.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #37044      +/-   ##
==========================================
- Coverage   79.60%   79.57%   -0.03%     
==========================================
  Files        2250     2250              
  Lines      211890   211951      +61     
==========================================
- Hits       168676   168668       -8     
- Misses      37540    37608      +68     
- Partials     5674     5675       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 10, 2025

Interestingly much of the code stated as not covered in the patch is covered in the e2e tests, I suspect that they don't contribute to the coverage since it's technically a different Go module?

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 10, 2025

@mx-psi, @codeboten, sorry to ping both of you, but if you have a moment in your day I'd love to get your thoughts on this implementation.

I did what I could to create a sane first API to maintain and use. Any suggestions are welcome! 😄

I did consider using Options hooks so it would look something like:

Start(
    WithEndpoint("localhost:1234"),
    WithRunDuration(5 * time.Second),
)

But I thought it wouldn't give much more flexibility than just:

mc := metrics.NewConfig()
mc.Endpoint = "localhost:1234"
mc.RunDuration = 5 * time.Second
err := metrics.Start(mc)

Which was simpler to implement and support

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 13, 2025

CI runner failure, but things seem to be running alright. Interestingly I don't have permissions to re-run the failed jobs.

@Erog38 Erog38 force-pushed the feat/export-telemetrygen-funcs-for-tests branch from f28d928 to 9c590e0 Compare January 15, 2025 23:27
@Erog38
Copy link
Contributor Author

Erog38 commented Jan 23, 2025

/easycla

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 24, 2025

/easycla

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 30, 2025

@codeboten Sorry to probe you on this, Just hoping to get your eyes on it since it's been requested by @mx-psi as well.

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 31, 2025

oh Thank you!

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 31, 2025

There appears to be an unrelated CI failure, @dmitryax is there a recommended way for users without permissions to kick the action again other than am empty commit?

@Erog38
Copy link
Contributor Author

Erog38 commented Jan 31, 2025

oh I see this issue is on main 😅

@mx-psi mx-psi merged commit 95d50af into open-telemetry:main Feb 5, 2025
163 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 5, 2025
@Erog38
Copy link
Contributor Author

Erog38 commented Feb 5, 2025

@mx-psi you're amazing thank you!

chengchuanpeng pushed a commit to chengchuanpeng/opentelemetry-collector-contrib that referenced this pull request Feb 8, 2025
…lemetry#37044)

#### Description

This PR addresses open-telemetry#36984 in order to open up telemetrygen to be used by
golang tests and generate varying amounts of telemetry in code to
support use cases which don't rely on external tools.

The following major changes were made to clear the footprint for how
this API can be used:

1. Moved `metrics`, `traces`, and `logs` under `pkg` instead of
internal.
2. Unexported the `run` function for each test suite so the entrypoint
is only the `Start` function
3. Moved config defaults from pflags into a `SetDefaults` function which
is exported alongside `NewConfig`
for changed packages in order for users to get the same sane defaults as
if run from the command line.
4. Adjusted the pflags to use the existing default values rather than
duplicate their own removing default config duplication
5. Added E2E tests for Metrics and Logs and adjusted Traces to use the
new API so it stays consistent.
6. Adjusted log and metric exporter instantiation slightly due to a race
condition with an `err` variable which was only caught by the new E2E
tests.

#### Testing

Added to the E2E tests to ensure consistent API specs.
zeck-ops pushed a commit to zeck-ops/opentelemetry-collector-contrib that referenced this pull request Apr 23, 2025
…lemetry#37044)

#### Description

This PR addresses open-telemetry#36984 in order to open up telemetrygen to be used by
golang tests and generate varying amounts of telemetry in code to
support use cases which don't rely on external tools.

The following major changes were made to clear the footprint for how
this API can be used:

1. Moved `metrics`, `traces`, and `logs` under `pkg` instead of
internal.
2. Unexported the `run` function for each test suite so the entrypoint
is only the `Start` function
3. Moved config defaults from pflags into a `SetDefaults` function which
is exported alongside `NewConfig`
for changed packages in order for users to get the same sane defaults as
if run from the command line.
4. Adjusted the pflags to use the existing default values rather than
duplicate their own removing default config duplication
5. Added E2E tests for Metrics and Logs and adjusted Traces to use the
new API so it stays consistent.
6. Adjusted log and metric exporter instantiation slightly due to a race
condition with an `err` variable which was only caught by the new E2E
tests.

#### Testing

Added to the E2E tests to ensure consistent API specs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/telemetrygen telemetrygen command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants