Cleanup after the Otel upgrade #4359

ptodev · 2023-07-06T14:26:03Z

PR Description

#3858 updated to version 0.80 of Otel Collector, but some work was still yet to be done. This PR completes the Otel upgrade by doing the following:

Updates our components to use the latest features of Collector's components
Disables Otel Collector "process" metrics
Uses version v1.0.0-rcv0013 of pdata and featuregate modules instead of the ones from the fork.

This PR completes the upgrade to version 0.80 of the Collector. No more work is expected to be needed.

PR Checklist

CHANGELOG updated
Documentation added
Tests updated

clayton-cornell

Two suggestions.

For not syntax, we should migrate to the correct markdown as we come across them in the topics. It's fine to leave existing notes as-is for now.

clayton-cornell · 2023-07-06T18:59:04Z

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

+> **NOTE**: `otelcol.exporter.jaeger` is a wrapper over the 
+> [upstream](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/jaegerexporter) 
+> OpenTelemetry Collector `jaeger` exporter. The upstream
+> exporter has been deprecated and will be removed from future versions of 
+> both OpenTelemetry Collector and Grafana Agent because Jaeger supports OTLP directly.


Suggested change

> **NOTE**: `otelcol.exporter.jaeger` is a wrapper over the

> [upstream](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/jaegerexporter)

> OpenTelemetry Collector `jaeger` exporter. The upstream

> exporter has been deprecated and will be removed from future versions of

> both OpenTelemetry Collector and Grafana Agent because Jaeger supports OTLP directly.

{{% admonition type="note" %}}

`otelcol.exporter.jaeger` is a wrapper over the [upstream](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/exporter/jaegerexporter) OpenTelemetry Collector `jaeger` exporter.

The upstream exporter has been deprecated and will be removed from future versions of both OpenTelemetry Collector and Grafana Agent because Jaeger supports OTLP directly.

{{% /admonition %}}

We use the > **NOTE**: syntax in all the Flow Otel pages. I would be happy to change it, but it'd be best to do it for all pages at once so we are consistent. Would you like me to raise a separate issue for it? It would also be nice to know why the {{% admonition type="note" %}} syntax is preferable?

The > **NOTE**: syntax formats the note as a blockquote. This works, but it relies on the blockquote formatting in the stylesheets staying constant and as expected.

The shortcode {{% admonition type="note" %}} ensures that ALL notes are formatted the same regardless of where they are in the docs... same with warnings and cautions... and the stylesheet is easier to manage.

Refer also to https://grafana.com/docs/writers-toolkit/write/shortcodes/#admonition-shortcode for formatting of the admonition shortcode.

So yes, if it's in the template we should update it.

If we leave it as-is in this PR, that's fine. We should plan to update the template for future

docs/sources/flow/reference/components/otelcol.processor.batch.md

rfratto

Thanks!

rfratto · 2023-07-11T14:53:47Z

component/otelcol/auth/headers/headers.go

+func (a *Action) UnmarshalText(text []byte) error {
+	str := Action(strings.ToLower(string(text)))
+	switch str {
+	case ActionInsert, ActionUpdate, ActionUpsert, ActionDelete:
+		*a = str
+		return nil
+	default:
+		return fmt.Errorf("unknown action %v", str)
+	}
+}


Does this need to exist or is the Validate method enough?

UnmarshalText needs to be implemented because otherwise River won't be able to unmarshal Action. Since I already had to implement it, I thought I might as well verify if it's correct since it doesn't make sense to unmarshal to an incorrect value.

UnmarshalText needs to be implemented because otherwise River won't be able to unmarshal Action.

Are you sure? Action is just a string.

Ah, I see, there's text transformation going on when unmarshaling to make it case insensitive. OK, that adds some validity to UnmarshalText but it's still not strictly necessary.

I'm ok with keeping it in both cases (including the tail sampling case being discussed below)

The text transformation isn't the main reason. If I don't implement UnmarshalText, I get this error:

panic: reflect.Set: value of type string is not assignable to type headers.Action [recovered] panic: reflect.Set: value of type string is not assignable to type headers.Action

The text transformation was simply so that we match what the Collector does. TBH I'm not 100% sure if I like it or if we should just stick to some Agent convention, but I wasn't sure if we have a convention and thought this way it'll be easier to migrate Collector configs to the Agent.

rfratto · 2023-07-11T14:54:47Z

component/otelcol/processor/tail_sampling/types.go

+func (e *ErrorMode) UnmarshalText(text []byte) error {
+	str := ErrorMode(strings.ToLower(string(text)))
+	switch str {
+	case ErrorModeIgnore, ErrorModePropagate:
+		*e = str
+		return nil
+	default:
+		return fmt.Errorf("unknown error mode %v", str)
+	}
+}


Does this need to exist or is the Validate method enough?

This is the same as the Action type in the other file in this PR.

UnmarshalText needs to be implemented because otherwise River won't be able to unmarshal ErrorMode. Since I already had to implement it, I thought I might as well verify if it's correct since it doesn't make sense to unmarshal to an incorrect value.

rfratto · 2023-07-11T14:55:23Z

component/otelcol/processor/tail_sampling/types.go

+// OttlConditionCfg holds the configurable setting to create a OTTL condition filter
+// sampling policy evaluator.
+type OttlConditionCfg struct {


The name here feels misaligned with our normal naming convention; this would usually be OTTLConditionConfig.

Ok, I changed the name of this type and all the other types in this file (they all used to end with Cfg)

rfratto · 2023-07-11T14:56:53Z

component/otelcol/receiver/jaeger/jaeger.go

@@ -30,8 +29,7 @@ func init() {

 // Arguments configures the otelcol.receiver.jaeger component.
 type Arguments struct {
-	Protocols      ProtocolsArguments       `river:"protocols,block"`
-	RemoteSampling *RemoteSamplingArguments `river:"remote_sampling,block,optional"`


This is a breaking change, right? It should be documented as such in the changelog.

That's true, but I didn't list it as a breaking change because it's not documented in the docs. Would you agree?
I know that it's technically a breaking change, but since it's undocumented I think we could make an exception.
Btw I raised a similar PR upstream.

Ah, sure, since it's not documented I agree it's safe to remove without a breaking change notice.

There was a discussion on this in the upstream PR, and I'm going to backtrack and just add an entry in the upgrade guide and in the changelog 😅

rfratto · 2023-07-11T14:57:25Z

CHANGELOG.md

@@ -23,11 +23,36 @@ Main (unreleased)

 - `otelcol.exporter.loki` now includes the instrumentation scope in its output. (@ptodev)

- `otelcol.extension.jaeger_remote_sampling` removes the `\` HTTP endpoint. The `/sampling` endpoint is still functional.
+- `otelcol.extension.jaeger_remote_sampling` removes the `/` HTTP endpoint. The `/sampling` endpoint is still functional.


Note that all breaking changes need to be documented with details and potential migration instructions in the upgrade guide.

Thank you, I forgot to update the guide in #3858. I added three entries to it now!

thampiotr

I don't have much context, but it looks good to me from the outside 👍

go.mod

Co-authored-by: Clayton Cornell <[email protected]>

ptodev · 2023-07-12T21:42:15Z

component/otelcol/processor/tail_sampling/types.go

+	if e == nil || *e == "" {
+		return ottl.ErrorMode("")
+	}


@rfratto - after your review I had to fix a linter issue, and I added this clause. It feels dodgy because ottl.ErrorMode("") is not a valid error mode, but I can't think of a better way to handle this. I'm just going to merge it as it is, I hope you don't mind.

* Cleanup after the Otel upgrade * Update docs/sources/flow/reference/components/otelcol.processor.batch.md Co-authored-by: Clayton Cornell <[email protected]> * PR review improvements * Update comment * Fix linter issue, document remote_sampling change --------- Co-authored-by: Clayton Cornell <[email protected]>

ptodev requested review from a team and clayton-cornell as code owners July 6, 2023 14:26

clayton-cornell reviewed Jul 6, 2023

View reviewed changes

ptodev force-pushed the upgrade-otel-cleanup2 branch from 5b8b935 to 3b2ae1f Compare July 11, 2023 07:43

ptodev requested a review from clayton-cornell July 11, 2023 07:47

rfratto approved these changes Jul 11, 2023

View reviewed changes

thampiotr approved these changes Jul 12, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

ptodev and others added 4 commits July 12, 2023 18:33

Cleanup after the Otel upgrade

0199d8b

Update docs/sources/flow/reference/components/otelcol.processor.batch.md

9dc069b

Co-authored-by: Clayton Cornell <[email protected]>

PR review improvements

6434e90

Update comment

d4377d4

ptodev force-pushed the upgrade-otel-cleanup2 branch from 46d1d2f to d4377d4 Compare July 12, 2023 15:38

Fix linter issue, document remote_sampling change

d06c667

ptodev force-pushed the upgrade-otel-cleanup2 branch from d67af11 to d06c667 Compare July 12, 2023 21:19

ptodev commented Jul 12, 2023

View reviewed changes

ptodev merged commit 1b7e538 into main Jul 12, 2023

ptodev deleted the upgrade-otel-cleanup2 branch July 12, 2023 21:42

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 Feb 22, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup after the Otel upgrade #4359

Cleanup after the Otel upgrade #4359

ptodev commented Jul 6, 2023 •

edited

Loading

clayton-cornell left a comment

clayton-cornell Jul 6, 2023

ptodev Jul 11, 2023

clayton-cornell Jul 12, 2023

clayton-cornell Jul 12, 2023

rfratto left a comment

rfratto Jul 11, 2023

ptodev Jul 11, 2023

rfratto Jul 11, 2023

rfratto Jul 11, 2023

ptodev Jul 12, 2023

rfratto Jul 11, 2023

ptodev Jul 11, 2023

rfratto Jul 11, 2023

ptodev Jul 11, 2023

rfratto Jul 11, 2023

ptodev Jul 11, 2023

rfratto Jul 11, 2023

ptodev Jul 12, 2023

rfratto Jul 11, 2023

ptodev Jul 11, 2023

thampiotr left a comment

ptodev Jul 12, 2023

Cleanup after the Otel upgrade #4359

Cleanup after the Otel upgrade #4359

Conversation

ptodev commented Jul 6, 2023 • edited Loading

PR Description

PR Checklist

clayton-cornell 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

rfratto 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

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

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

Choose a reason for hiding this comment

thampiotr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptodev commented Jul 6, 2023 •

edited

Loading