Upgrade OTel from v0.87.0 to v0.96.0 #6725

ptodev · 2024-03-19T13:49:22Z

PR Description

OTel upgrade. I went through the changes in the OTel changelog. As far as I can tell, this PR updates all the relevant Agent bits. I hope I didn't miss anything.

Which issue(s) this PR fixes

Fixes #6389

Notes to the Reviewer

I tested this locally with the Static and River files according to our docs.

There are some additional things I could do:

Add tests for the new config arguments.
There are a few TODOs in the changelog which I need to take a look at.
When the fork PR is merged, I need to update the go.mod file again.
Update config converters?

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

wildum

Thanks for taking care of this! I only have a few comments, nothing major. With this review I only checked the added code. I did not go through all opentelemetry release changes to see if something was missing

wildum · 2024-03-20T10:18:41Z

docs/sources/flow/reference/components/otelcol.connector.spanmetrics.md

+| `metrics_flush_interval`          | `duration`     | How often to flush generated metrics.                                                      | `"15s"`        | no       |
+| `namespace`                       | `string`       | Metric namespace.                                                                          | `""`           | no       |
+| `resource_metrics_cache_size`     | `number`       | The size of the cache holding metrics for a service.                                       | `1000`         | no       |
+| `resource_metrics_key_attributes` | `list(string)` | Span resources with the same values for those resource attributes are aggregated together. | `[]`           | no       |


nit: I did not find this sentence very clear "Span resources with the same values for those resource attributes are aggregated together."

docs/sources/flow/reference/components/otelcol.connector.spanmetrics.md

internal/component/otelcol/connector/spanmetrics/types.go

wildum · 2024-03-20T10:42:38Z

CHANGELOG.md

+  * [otelcol.connector.spanmetrics] Add a new `events` metric.
+https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27451
+  * [otelcol.connector.spanmetrics] A new `max_per_data_point` argument for exemplar generation.
+  * https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/22620


I think that this link is wrong, it should be open-telemetry/opentelemetry-collector-contrib#29242

docs/sources/flow/reference/components/otelcol.exporter.otlp.md

docs/sources/flow/reference/components/otelcol.exporter.otlphttp.md

wildum · 2024-03-20T10:54:44Z

internal/component/otelcol/exporter/otlphttp/otlphttp.go

@@ -93,6 +102,9 @@ func (args *Arguments) Validate() error {
 	if args.Client.Endpoint == "" && args.TracesEndpoint == "" && args.MetricsEndpoint == "" && args.LogsEndpoint == "" {
 		return errors.New("at least one endpoint must be specified")
 	}
+	if args.Encoding != EncodingProto && args.Encoding != EncodingJSON {
+		return errors.New("invalid encoding type")


should the error also say what the valid types are?

docs/sources/flow/reference/components/otelcol.exporter.otlphttp.md

wildum · 2024-03-20T10:59:56Z

docs/sources/flow/reference/components/otelcol.exporter.otlphttp.md

+`max_conns_per_host`      | `int`                      | Limits the total (dialing,active, and idle) number of connections per host. | `0` | no
+`idle_conn_timeout`       | `duration`                 | Time to wait before an idle connection closes itself. | `"90s"` | no
+`disable_keep_alives`     | `bool`                     | Disable HTTP keep-alive. | `false` | no
+`http2_read_idle_timeout` | `duration`                 | Timeout after which a health check using ping frame will be carried out if no frame is received on the connection. | `0a` | no


Maybe we could also add that if the timeout is unset or set to 0, then no health check will be performed

internal/component/otelcol/connector/servicegraph/servicegraph.go

wildum · 2024-03-20T13:59:42Z

Found a missing change: in extract-field-block.md we say that "The source of the labels or annotations. Allowed values are pod and namespace." But this change in v0.89.0 adds the value "node": open-telemetry/opentelemetry-collector-contrib#28570

wildum · 2024-03-20T16:22:03Z

Scrolled through the rest of the open telemetry contrib release, I did not see any other missing changes, well done 👍 For the future I would recommend having atomic commits for every change to make the review easier

ptodev · 2024-03-21T10:43:04Z

@wildum Thank you for the review! I addressed some of your comments. I plan to address the other ones in a subsequent PR.

I also took out these lines from the CHANGELOG:

  * [otelcol.connector.servicegraph] Additional debug metrics
  <!-- TODO: Document those metrics -->
https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/29917
  * [otelcol.processor.filter] Additional debug metrics
  <!-- TODO: Document those metrics -->
https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/30736
  * Scope name for all generated Meter/Tracer funcs now includes full package name
  <!-- TODO: Does this affect the Agent's metrics and logs? -->
https://github.com/open-telemetry/opentelemetry-collector/issues/9494
  * [otelcol.connector.servicegraph] Measure latency in seconds instead of milliseconds
  <!-- TODO: This is a breaking change for Static mode? -->
  <!-- TODO: Is this a breaking change for Flow? -->
https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27488

I will review them and reintroduce them in a subsequent PR.

wildum

Second review, only two little comments

wildum · 2024-03-21T11:00:40Z

internal/component/otelcol/config_tls.go

@@ -81,6 +82,7 @@ func (args *TLSSetting) Convert() *otelconfigtls.TLSSetting {
 		MinVersion:               args.MinVersion,
 		MaxVersion:               args.MaxVersion,
 		ReloadInterval:           args.ReloadInterval,
+		CipherSuites:             args.CipherSuites,


should we create a new slice instead of copying it like this?

wildum · 2024-03-21T11:11:12Z

internal/static/traces/config_test.go

@@ -1159,6 +1159,8 @@ remote_write:
      scopes: ["api.metrics"]
      timeout: 2s
 `,
+			//TODO(ptodev): Look into why we need to add a "cipher_suites: []" explicitly.
+			// The expected config should unmarshal it to [], but instead it sets it to nil.


It seems correct to me that the config would unmarshal it to nil. Else maybe you can explicitly create a list during the convert part (converting from nil to {}string[]). I think that we should avoid adding "cipher_suites: []" explicitly

It seems correct to me that the config would unmarshal it to nil.

I used a debugger in one of the Collector's unit tests, to see if it's nil or []. It was [], so I assumed that this is the correct thing to do. I don't know if the unit tests deviates from their prod code path, but I guess it doesn't.

I think the way Agent's unit test creates the Collector config isn't the same way the Collector does it. Something is different, but I'm not sure what exactly.

wildum

let's ship it!

wildum · 2024-03-22T09:28:31Z

@clayton-cornell could you have a look at the docs when you have time? We are merging the branch because this PR is already very big and we don't want it to be stuck longer. Some of my comments and your future comments will be addressed in a subsequent PR

ptodev requested a review from clayton-cornell as a code owner March 19, 2024 13:49

ptodev force-pushed the ptodev/update-otel branch from 8f97b37 to a1d9f72 Compare March 19, 2024 13:54

wildum self-assigned this Mar 20, 2024

wildum reviewed Mar 20, 2024

View reviewed changes

ptodev force-pushed the ptodev/update-otel branch from a1d9f72 to 40a289a Compare March 21, 2024 10:43

wildum reviewed Mar 21, 2024

View reviewed changes

ptodev force-pushed the ptodev/update-otel branch from 4c7c23a to 831b807 Compare March 21, 2024 17:19

tpaschalis mentioned this pull request Mar 21, 2024

otelcol: panic when fanning out to invalid receiver block #6746

Closed

ptodev force-pushed the ptodev/update-otel branch from 831b807 to 8f0d269 Compare March 21, 2024 17:30

ptodev added 4 commits March 22, 2024 09:18

Upgrade OTel from v0.87.0 to v0.96.0

c38b047

Fixes for linter and unit tests

46ffa11

Use the grafana fork of the Collector

cb6e143

Fix issues with converters

636041f

ptodev force-pushed the ptodev/update-otel branch from 8f0d269 to 636041f Compare March 22, 2024 09:18

wildum approved these changes Mar 22, 2024

View reviewed changes

ptodev merged commit da363b7 into main Mar 22, 2024
10 checks passed

ptodev deleted the ptodev/update-otel branch March 22, 2024 09:34

github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade OTel from v0.87.0 to v0.96.0 #6725

Upgrade OTel from v0.87.0 to v0.96.0 #6725

ptodev commented Mar 19, 2024

wildum left a comment

wildum Mar 20, 2024

wildum Mar 20, 2024

wildum Mar 20, 2024

wildum Mar 20, 2024

wildum commented Mar 20, 2024

wildum commented Mar 20, 2024

ptodev commented Mar 21, 2024

wildum left a comment

wildum Mar 21, 2024

wildum Mar 21, 2024

ptodev Mar 21, 2024

ptodev Mar 21, 2024

wildum left a comment

wildum commented Mar 22, 2024

Upgrade OTel from v0.87.0 to v0.96.0 #6725

Upgrade OTel from v0.87.0 to v0.96.0 #6725

Conversation

ptodev commented Mar 19, 2024

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

wildum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildum commented Mar 20, 2024

wildum commented Mar 20, 2024

ptodev commented Mar 21, 2024

wildum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildum left a comment

Choose a reason for hiding this comment

wildum commented Mar 22, 2024