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

[cmd/telemetrygen] create a simple counter metric generator #17898

Merged
merged 9 commits into from
Feb 15, 2023

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jan 21, 2023

Description:
Basic telemetrygen addition for metrics

Link to tracking Issue:
#17986

Testing:
Unit tests.

Documentation:
None

@github-actions github-actions bot added the cmd/telemetrygen telemetrygen command label Jan 21, 2023
@runforesight
Copy link

runforesight bot commented Jan 21, 2023

Foresight Summary

    
Major Impacts

build-and-test-windows duration(4 seconds) has decreased 40 minutes 56 seconds compared to main branch avg(41 minutes).
View More Details

✅  tracegen workflow has finished in 3 minutes 2 seconds and finished at 25th Jan, 2023.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details

✅  check-links workflow has finished in 44 seconds (1 minute 50 seconds less than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  telemetrygen workflow has finished in 59 seconds (2 minutes 7 seconds less than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  changelog workflow has finished in 2 minutes 26 seconds (1 minute 17 seconds less than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 11 minutes 23 seconds (⚠️ 2 minutes 31 seconds more than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
prometheus-compliance-tests -     🔗  ✅ 21  ❌ 0  ⏭ 0    🔗 See Details

✅  load-tests workflow has finished in 22 minutes 29 seconds (⚠️ 5 minutes 30 seconds more than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
loadtest (TestIdleMode) -     🔗  ✅ 1  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  ✅ 6  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  ✅ 18  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  ✅ 12  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  ✅ 10  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceAttributesProcessor) -     🔗  ✅ 3  ❌ 0  ⏭ 0    🔗 See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  ✅ 8  ❌ 0  ⏭ 0    🔗 See Details
setup-environment -     🔗  N/A See Details

✅  e2e-tests workflow has finished in 15 minutes 21 seconds and finished at 15th Feb, 2023.


Job Failed Steps Tests
kubernetes-test -     🔗  N/A See Details

✅  build-and-test workflow has finished in 44 minutes 56 seconds (20 minutes 38 seconds less than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
unittest-matrix (1.20, internal) -     🔗  ✅ 561  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, processor) -     🔗  ✅ 1528  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, internal) -     🔗  ✅ 561  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, extension) -     🔗  ✅ 537  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, processor) -     🔗  ✅ 1528  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-0) -     🔗  ✅ 2574  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-0) -     🔗  ✅ 2574  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, exporter) -     🔗  ✅ 2455  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, exporter) -     🔗  ✅ 2455  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, other) -     🔗  ✅ 4687  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.20, receiver-1) -     🔗  ✅ 1928  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, receiver-1) -     🔗  ✅ 1928  ❌ 0  ⏭ 0    🔗 See Details
unittest-matrix (1.19, other) -     🔗  ✅ 4687  ❌ 0  ⏭ 0    🔗 See Details
unittest (1.20) -     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) -     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details

⭕  build-and-test-windows workflow has finished in 4 seconds (40 minutes 56 seconds less than main branch avg.) and finished at 15th Feb, 2023.


Job Failed Steps Tests
windows-unittest-matrix -     🔗  N/A See Details
windows-unittest -     🔗  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@atoulme atoulme force-pushed the telemetrygen_metrics branch 2 times, most recently from 5df371e to 0331305 Compare January 24, 2023 06:23
@atoulme atoulme marked this pull request as ready for review January 24, 2023 06:24
@atoulme atoulme requested a review from a team January 24, 2023 06:24
@atoulme atoulme force-pushed the telemetrygen_metrics branch from 0331305 to 5a08b26 Compare January 24, 2023 07:44
Copy link
Member

@mx-psi mx-psi 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 the PR! My main comment is that we should refactor the implementation to reuse common bits across all subcommands. For me it would be easier to review this on a separate refactor-only PR that we merge first, but we can also do it here if the separation is clean enough :)

cmd/telemetrygen/internal/metrics/config.go Outdated Show resolved Hide resolved
cmd/telemetrygen/internal/metrics/config.go Show resolved Hide resolved
cmd/telemetrygen/internal/metrics/config.go Outdated Show resolved Hide resolved
cmd/telemetrygen/internal/metrics/metrics.go Outdated Show resolved Hide resolved
cmd/telemetrygen/internal/metrics/metrics.go Outdated Show resolved Hide resolved
@atoulme
Copy link
Contributor Author

atoulme commented Jan 24, 2023

I'll work on this, good points raised. You might see me apply your comments to the trace counterpart as I mostly... copied that code, as you inferred ;)

@atoulme atoulme requested review from mx-psi January 25, 2023 08:07
@atoulme atoulme force-pushed the telemetrygen_metrics branch 2 times, most recently from 577060c to f3c3c5d Compare January 25, 2023 20:33
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Code LGTM, thanks for doing the refactor, I think it will be much easier to maintain the code this way!

My only remaining comment/concern is if OTLP sums should be the default type for the metrics subcommand or whether we should produce OTLP Gauges instead. I lean towards the latter because Gauge feels like the 'simplest' type (no aggregation temporality, no need for start timestamp, simpler semantics...) but I want to hear your thoughts on this too.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 30, 2023

Oh, I assumed we'd want to add more options and support multiple metric types, but I like the idea of a gauges command I misread you. Yes, we can use whatever is sensible here. I see this as a first PR, there's more work on this needed.

@atoulme atoulme force-pushed the telemetrygen_metrics branch 2 times, most recently from c3f3a1c to bcf78c5 Compare January 31, 2023 06:37
@mx-psi
Copy link
Member

mx-psi commented Jan 31, 2023

I definitely envisioned this as supporting all OTLP metric types, I was just talking about the default :)

@atoulme
Copy link
Contributor Author

atoulme commented Feb 9, 2023

I don't see how to use a gauge because as described, it reports one data point per measurement collection cycle:

// Int64ObservableGauge returns a new instrument identified by name and
	// configured with options. The instrument is used to asynchronously record
	// instantaneous int64 measurements once per a measurement collection
	// cycle.
	Int64ObservableGauge(name string, options ...instrument.Int64ObserverOption) (instrument.Int64ObservableGauge, error)

Trying to go for a counter instead, but the points are summarized to the last value. I need to override the setup to report all points.

@atoulme atoulme force-pushed the telemetrygen_metrics branch from 359263d to 21bf349 Compare February 14, 2023 07:02
@atoulme atoulme requested a review from mx-psi February 15, 2023 07:23
@mx-psi mx-psi merged commit 7462050 into open-telemetry:main Feb 15, 2023
@atoulme atoulme deleted the telemetrygen_metrics branch February 15, 2023 16:24
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.

4 participants