Update OTEL dependencies to latest release candidates #620

lrascao · 2023-04-05T12:39:32Z

Relevant changes required from RC changelog:

Both the Histogram and HistogramDataPoint are redefined with a generic argument of [N int64 | float64] in go.opentelemetry.io/otel/sdk/metric/metricdata. (Add Exemplar to metricdata package open-telemetry/opentelemetry-go#3849)
The metric Export interface from go.opentelemetry.io/otel/sdk/metric accepts a *ResourceMetrics instead of ResourceMetrics. (Update the metric Export interface to accept a *ResourceMetrics instead of ResourceMetrics open-telemetry/opentelemetry-go#3853)

Also of note go.opentelemetry.io/otel/metric is now stable as of v1.15.0-rc2

fixes #620

codecov · 2023-04-05T12:44:03Z

Codecov Report

Merging #620 (f14e232) into main (478fe63) will increase coverage by 0.02%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main     #620      +/-   ##
==========================================
+ Coverage   66.25%   66.27%   +0.02%     
==========================================
  Files          36       36              
  Lines        4264     4276      +12     
==========================================
+ Hits         2825     2834       +9     
- Misses       1312     1314       +2     
- Partials      127      128       +1

Impacted Files	Coverage Δ
exporter/metric/metric.go	`71.00% <87.50%> (+0.10%)`	⬆️
.../collector/integrationtest/testcases/conversion.go	`76.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

exporter/metric/metric.go

damemi

Hi @lrascao, thanks for submitting this. However, we would like to wait until the Go libraries are released as stable rather than merge the rc branches (release candidates aren't technically stable, yet). There's some more discussion in a similar PR #611

If you don't mind waiting and updating this PR once these tags are published, that would be really helpful. Otherwise, we would like to hold off on merging rc dependencies unless necessary

lrascao · 2023-04-05T14:21:51Z

If you don't mind waiting and updating this PR once these tags are published, that would be really helpful.

sure, i'll move this to draft until then

ptxmac · 2023-05-08T09:01:10Z

v1.15 is tagged as stable now, any update on when this will be merged?

damemi · 2023-05-08T11:22:27Z

@ptxmac thanks for bumping this, now that 1.15 is stable we should merge it

@lrascao can you resolve the conflicts and switch this from draft to ready for review?

lrascao · 2023-05-08T20:17:01Z

Looks like we need to wait for folks at https://github.com/open-telemetry/opentelemetry-collector to cut a new release that also bumps to v1.15.0

ptxmac · 2023-05-09T08:10:34Z

Looks like we need to wait for folks at https://github.com/open-telemetry/opentelemetry-collector to cut a new release that also bumps to v1.15.0

Looks like they did just one hour after your post 😄

lrascao · 2023-05-09T10:19:24Z

Apparently there's a regression in the google managed prometheus metric naming convention:

--- FAIL: TestGetMetricName (0.00s)
    --- FAIL: TestGetMetricName/sum_without_total (0.00s)
        naming_test.go:143: MetricName(foo, {orig:0xc0001d5080})=foo_total/counter; want foo/counter
    --- FAIL: TestGetMetricName/sum_with_unit (0.00s)
        naming_test.go:143: MetricName(foo, {orig:0xc0001d5100})=foo_seconds_total/counter; want foo_total/counter
FAIL
FAIL    github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/collector/googlemanagedprometheus   0.239s

damemi · 2023-05-15T13:49:08Z

@lrascao is this still a draft or is it ready for review? Not sure if you were working on the regression you mentioned in your last comment

damemi · 2023-05-15T13:56:40Z

Apparently there's a regression in the google managed prometheus metric naming convention:

--- FAIL: TestGetMetricName (0.00s)
    --- FAIL: TestGetMetricName/sum_without_total (0.00s)
        naming_test.go:143: MetricName(foo, {orig:0xc0001d5080})=foo_total/counter; want foo/counter
    --- FAIL: TestGetMetricName/sum_with_unit (0.00s)
        naming_test.go:143: MetricName(foo, {orig:0xc0001d5100})=foo_seconds_total/counter; want foo_total/counter
FAIL
FAIL    github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/collector/googlemanagedprometheus   0.239s

Digging around it looks like this is caused by open-telemetry/opentelemetry-collector-contrib#20519. Looking into it

lrascao · 2023-05-15T15:09:55Z

@lrascao is this still a draft or is it ready for review? Not sure if you were working on the regression you mentioned in your last comment

yeah, not really sure on that path forward here, adapt to the change or request rollback of the regression

damemi · 2023-05-15T15:11:49Z

Talked to the team, and it looks like we should accept the change in default naming to match the Prometheus specification. In other words, just update the failing tests to expect the new names (with _total suffix).

This will be a breaking change, but it can be disabled using the feature gate flag from the upstream change (--feature-gates=-pkg.translator.prometheus.NormalizeName).

lrascao · 2023-05-15T15:13:37Z

Talked to the team, and it looks like we should accept the change in default naming to match the Prometheus specification. In other words, just update the failing tests to expect the new names (with _total suffix).

This will be a breaking change, but it can be disabled using the feature gate flag from the upstream change (--feature-gates=-pkg.translator.prometheus.NormalizeName).

alright, updating

go.opentelemetry.io/otel v1.15.1/v0.38.1 go.opentelemetry.io/contrib v1.16.0/v0.41.0 go.opentelemetry.io/collector v1.0.0/v0.76.1 Signed-off-by: Luis Rascao <luis.rascao@gmail.com>

lrascao · 2023-05-15T15:41:57Z

ping @damemi ready for a GCB run

damemi · 2023-05-15T16:02:30Z

/gcbrun

lrascao · 2023-05-15T16:12:21Z

/gcbrun

tests need a retry it seems

damemi

just want to add that test back

e2e-test-server/cloud_functions/go.mod

exporter/collector/googlemanagedprometheus/naming_test.go

damemi · 2023-05-17T16:03:39Z

I just found open-telemetry/opentelemetry-collector-contrib#21743, which also discusses the changes in metric name normalization. There might be some upstream work to do with this first

exporter/metric/metric.go

ktong · 2023-05-23T01:38:55Z

Could it upgrade to v1.16.0/v0.39.0 instead to avoid another upgrade? https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.16.0

damemi · 2023-05-23T19:23:43Z

/gcbrun

damemi · 2023-05-23T19:32:32Z

Could it upgrade to v1.16.0/v0.39.0 instead to avoid another upgrade? https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.16.0

We are pretty close with this one, so given the number of changes I think it would be best to update incrementally if possible. So, getting this green then immediately following with another bump would be better imo.

Looking into why some of the unit tests are failing

exporter/metric/metric.go

exporter/collector/integrationtest/metrics_test.go

damemi · 2023-05-25T13:59:16Z

/gcbrun

exporter/metric/metric.go

lrascao · 2023-05-26T13:33:13Z

@damemi needs a splash of /gcbrun

damemi · 2023-05-26T14:30:10Z

/gcbrun

damemi

Thanks for all of your help @lrascao!

damemi · 2023-05-26T16:32:09Z

exporter/collector/integrationtest/testcases/conversion.go

@@ -152,13 +152,13 @@ func convertHistogram(h pmetric.Histogram) metricdata.Aggregation {
 	if h.DataPoints().Len() == 0 {
 		return nil
 	}
-	agg := metricdata.Histogram{
+	agg := metricdata.Histogram[float64]{


As a note, I think it is fine that we are only handling float64 histograms here, because

this code is just handling testcases.

Sum is the only value used here that can be int or float in metricdata.Histogram[N] (we don't read Min or Max) and Sum() always returns a float64, so there shouldn't be any runtime issues with this.

We force float64 when we handle Sum anyway, which is consistent with the behavior prior to this update and therefore not breaking.

However, we should follow this up with support for int64 histograms. Otherwise, users who switch to the GCP exporter might see slightly different values since we force floats in the output calculation. That would include adding a test case for an int64 histogram, which we don't appear to have currently. Will make a separate issue to track this.

lrascao commented Apr 5, 2023

View reviewed changes

exporter/metric/metric.go Show resolved Hide resolved

lrascao marked this pull request as ready for review April 5, 2023 12:51

lrascao requested a review from a team as a code owner April 5, 2023 12:51

damemi reviewed Apr 5, 2023

View reviewed changes

lrascao marked this pull request as draft April 5, 2023 14:22

lrascao force-pushed the bump-otel branch from dec5cea to 639360d Compare May 8, 2023 18:23

lrascao force-pushed the bump-otel branch from 639360d to 66c85a8 Compare May 9, 2023 10:17

lrascao force-pushed the bump-otel branch from 66c85a8 to 4be09c8 Compare May 10, 2023 13:40

damemi mentioned this pull request May 15, 2023

Align with new Otel metrics release #629

Closed

damemi self-assigned this May 15, 2023

Update OTEL dependencies to latest release candidates

9292c82

go.opentelemetry.io/otel v1.15.1/v0.38.1 go.opentelemetry.io/contrib v1.16.0/v0.41.0 go.opentelemetry.io/collector v1.0.0/v0.76.1 Signed-off-by: Luis Rascao <luis.rascao@gmail.com>

lrascao force-pushed the bump-otel branch from 4be09c8 to 2fe11cc Compare May 15, 2023 15:18

fixup! Update OTEL dependencies to latest release candidates

2d66fd4

lrascao force-pushed the bump-otel branch from 2fe11cc to 2d66fd4 Compare May 15, 2023 15:27

lrascao marked this pull request as ready for review May 15, 2023 15:41

damemi suggested changes May 15, 2023

View reviewed changes

e2e-test-server/cloud_functions/go.mod Show resolved Hide resolved

exporter/collector/googlemanagedprometheus/naming_test.go Show resolved Hide resolved

marvinkite reviewed May 19, 2023

View reviewed changes

exporter/metric/metric.go Show resolved Hide resolved

fixup! Update OTEL dependencies to latest release candidates

ff4e074

thall mentioned this pull request May 23, 2023

feat: bump opentelemetry dependencies einride/cloudrunner-go#453

Merged

damemi suggested changes May 24, 2023

View reviewed changes

exporter/metric/metric.go Show resolved Hide resolved

exporter/metric/metric.go Outdated Show resolved Hide resolved

exporter/metric/metric.go Outdated Show resolved Hide resolved

fixup! Update OTEL dependencies to latest release candidates

bad0d26

damemi reviewed May 25, 2023

View reviewed changes

exporter/collector/integrationtest/metrics_test.go Outdated Show resolved Hide resolved

damemi suggested changes May 25, 2023

View reviewed changes

exporter/metric/metric.go Outdated Show resolved Hide resolved

fixup! Update OTEL dependencies to latest release candidates

f14e232

lrascao requested review from ptxmac, damemi and marvinkite May 26, 2023 13:32

damemi approved these changes May 26, 2023

View reviewed changes

damemi mentioned this pull request May 26, 2023

Support int64 histograms #633

Closed

damemi merged commit 50e80b0 into GoogleCloudPlatform:main May 26, 2023

lrascao deleted the bump-otel branch May 29, 2023 09:40

This was referenced May 30, 2023

Prepare release v0.38.0/v1.14.0 #634

Merged

Update otel dependencies #635

Merged

damemi mentioned this pull request Jul 14, 2023

Bump opentelemetry dependencies #672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update OTEL dependencies to latest release candidates #620

Update OTEL dependencies to latest release candidates #620

lrascao commented Apr 5, 2023 •

edited by damemi

Loading

codecov bot commented Apr 5, 2023 •

edited

Loading

damemi left a comment

lrascao commented Apr 5, 2023

ptxmac commented May 8, 2023

damemi commented May 8, 2023

lrascao commented May 8, 2023

ptxmac commented May 9, 2023

lrascao commented May 9, 2023

damemi commented May 15, 2023

damemi commented May 15, 2023

lrascao commented May 15, 2023

damemi commented May 15, 2023

lrascao commented May 15, 2023

lrascao commented May 15, 2023

damemi commented May 15, 2023

lrascao commented May 15, 2023

damemi left a comment

damemi commented May 17, 2023

ktong commented May 23, 2023

damemi commented May 23, 2023

damemi commented May 23, 2023

damemi commented May 25, 2023

lrascao commented May 26, 2023

damemi commented May 26, 2023

damemi left a comment

damemi May 26, 2023

Update OTEL dependencies to latest release candidates #620

Update OTEL dependencies to latest release candidates #620

Conversation

lrascao commented Apr 5, 2023 • edited by damemi Loading

codecov bot commented Apr 5, 2023 • edited Loading

Codecov Report

damemi left a comment

Choose a reason for hiding this comment

lrascao commented Apr 5, 2023

ptxmac commented May 8, 2023

damemi commented May 8, 2023

lrascao commented May 8, 2023

ptxmac commented May 9, 2023

lrascao commented May 9, 2023

damemi commented May 15, 2023

damemi commented May 15, 2023

lrascao commented May 15, 2023

damemi commented May 15, 2023

lrascao commented May 15, 2023

lrascao commented May 15, 2023

damemi commented May 15, 2023

lrascao commented May 15, 2023

damemi left a comment

Choose a reason for hiding this comment

damemi commented May 17, 2023

ktong commented May 23, 2023

damemi commented May 23, 2023

damemi commented May 23, 2023

damemi commented May 25, 2023

lrascao commented May 26, 2023

damemi commented May 26, 2023

damemi left a comment

Choose a reason for hiding this comment

damemi May 26, 2023

Choose a reason for hiding this comment

lrascao commented Apr 5, 2023 •

edited by damemi

Loading

codecov bot commented Apr 5, 2023 •

edited

Loading