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

Flow: otelcol.processor.attributes component #2294

Closed
Tracked by #2213
rfratto opened this issue Oct 3, 2022 · 11 comments · Fixed by #3349
Closed
Tracked by #2213

Flow: otelcol.processor.attributes component #2294

rfratto opened this issue Oct 3, 2022 · 11 comments · Fixed by #3349
Assignees
Labels
flow Related to Grafana Agent Flow frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Milestone

Comments

@rfratto
Copy link
Member

rfratto commented Oct 3, 2022

Expose the OpenTelemtry Collector attributes processor as a Flow component called otelcol.processor.attributes. This component is from the otelcol-contrib distribution.

@ptodev
Copy link
Contributor

ptodev commented Dec 21, 2022

I got stuck on this project because I need to create instances of "filterconfig.MatchConfig" and "attraction.Settings" but they are internal:
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/attributesprocessor/config.go#L37

One way to do this is to fork the contrib repo and export all the internals, as we currently do with the non-contrib one. @gouthamve suggested to try exporting those and checking if the changes get accepted in the official contrib repo before we fork.

I forked the contrib repo in my private GitHub area and exported those in the pkg folder. This is on tag v0.0.900. However, then the Agent didn't compile because it was using an old version of OTEL which has a function FromRaw which does not return an error. FromRaw was changed to return an error here at version 0.65.0 of OTEL. I tried updating the version of OTEL which the Agent uses, but then there were other compilation problems.

Finally, I tried rolling back both my OTEL and contrib changes, and using version 0.61.0 of otel-contrib. But it turns out that this version has the filter library in an old, awkward location which would look strange if exported.

It seems to me that the only realistic way to move forward is to fork the contrib repo and export all internals. @tpaschalis mentioned that the exporting script we use for OTEL is this one, so maybe we can have some variation of this for contrib.

The main thing that bothers me about the forks is that OTEL could one day remove config attributes, or even change the meaning of some attributes. I guess in those cases clients would not be able to upgrade to new versions of the Agent until they change their OTEL config, and there would be only one version of OTEL that a given version of Agent supports.

By the way, while speaking with @kovrus he mentioned that a lot of OTEL components (such as the attributes processor) are implemented using the OTTL language. We could theoretically also use this language, although at the moment I guess we are just writing adaptors to already existing OTEL components. But what worries me about OTTL is that people might have a complicated OTTL config which they try to use within the Agent - then there would be a mixture of OTTL and River and that may look complicated.

@ptodev
Copy link
Contributor

ptodev commented Dec 23, 2022

I managed to build a fork of otel-contrib based on the 0.63.0 branch where the "internal" folder is renamed to "external":
https://github.com/ptodev/opentelemetry-collector-contrib/tree/external-v.0.63.0-attempt3

I also made a lot of progress with the attribute processor code - it's on the branch indicated on this issue. It's still not finished though... I will try to finish it when I get back in January.

It would be good of we can decide how we want to import otel-contrib. We could use fork where we make the internals external. But a cleaner approach could be to translate River to Yaml, and then use factories in the OTEL code which convert Yaml to the OTEL data structure. Then we can just use the data structure from there. If there are no such factories already, I think we could add them - such a change would be likely to be accepted to the OTEL repo? Another upside to this approach is that it would allow us to test the translation from River to Otel more easily. We could use configs like this in our unit tests to see if our River inputs match them.

I think we were also discussing that a translation between River and Yaml might be a requirement for the Agent to become a distribution, so it may be something we want to do anyway.

That said, a fork might be the only realistic short term solution.

@rfratto rfratto modified the milestones: v0.30.0, v0.31.0 Jan 3, 2023
@rfratto
Copy link
Member Author

rfratto commented Jan 4, 2023

In general, forks should only be used as temporary workarounds. As of the latest OpenTelemetry Collector release, it's possible for us to use nothing but public APIs for the pkg/traces subsystem, which will finally help us get rid of our fork.

However: some of the APIs we previously used no longer have exposed counterparts (not even if we export the internal packages), so this isn't a simple task. Similarly, we need the ability to pass a custom logger and meter provider to the collector package for us to be able to use it.

I see the following path here:

  1. Open a PR upstream to export the types we need to use for the attributes processor.
  2. Open a PR upstream to enable passing a custom logger and meter provider to the collector.
  3. Wait for a new OpenTelemetry release with the two PRs.
  4. With the new OpenTelemetry Release, rewrite the pkg/traces subsystem to use public APIs from the latest release.
  5. Implement otelcol.processor.attributes.

@rfratto rfratto moved this from Todo to Blocked in Grafana Agent (Public) Jan 9, 2023
@rfratto rfratto added flow Related to Grafana Agent Flow flow/feature-parity labels Jan 19, 2023
@rfratto rfratto modified the milestones: v0.31.0, v0.32.0 Jan 30, 2023
@rfratto rfratto moved this from Blocked to Todo in Grafana Agent (Public) Feb 6, 2023
@erikbaranowski
Copy link
Contributor

@rfratto suggested a workaround that did in fact work for this issue (thank you!). We used the otel mapstructure to decode our river types into otel types. There is some example code here showing the implementation.

tail_sampling.go
types.go

As a second optional consideration, I implemented the squashed otel type sharedPolicyCfg by skipping it in the river types and using good old copy and paste in the 3 river types that use it. You have this option if you don't want to have to touch user docs after river implements squashing. In short, the downside is the repeated code in types.go and the upside is not having to change the river config/docs later when river implements squashing.

@rfratto rfratto modified the milestones: v0.32.0, v0.33.0 Feb 28, 2023
@ptodev ptodev self-assigned this Mar 7, 2023
@ptodev
Copy link
Contributor

ptodev commented Mar 23, 2023

We used the otel mapstructure to decode our river types into otel types.

@erikbaranowski The blocker for this task is that some of the Otel types that we need to convert are internal. Hence, we cannot instantiate them in our code. The pattern from the tail sampling code does require us to instantiate the Otel type before calling mustDecodeMapStructure(...). Hence, I don't think we can apply the same technique here. Unless I am missing something?

The only way we could use mapstructure.Decode is if we could somehow make a map of the root level, public Otel type (in this case that's attributesprocessor.Config), and then assume that it can use the mapstructure info from Otel to reconstruct the Otel object. I don't know how mapstructure works exactly, but I doubt it would be able to do this when the Otel types are internal.

Another solution could be if we had a way to stream out the River config into a yaml, then maybe we could somehow convert that into Otel using some Otel/mapstructure factory. However, I still don't see how we could use mapstructure.Decode here, since we can't just pass a whole Yaml config to it?

@rfratto
Copy link
Member Author

rfratto commented Mar 23, 2023

The overall Config type from the processor is exported, so we can do mapstructure.Decode into it.

I doubt it would be able to do this when the Otel types are internal.

It can. mapstructure.Decode is how OpenTelemetry Collector decodes YAML configs, which is why OpenTelemetry Collector can create a config for that copmonent even though it has types from internal packages.

@ptodev
Copy link
Contributor

ptodev commented Mar 23, 2023

Thanks @rfratto. I suspected that this works for Otel because these internal types are available to them, and they call mapstructure from within Otel. But the types are not available to us, hence why I thought it wouldn't work.

Just now I tried this in a unit test and it built ok:

func Test(t *testing.T) {
	input := map[string]interface{}{
		"actions": []interface{}{
			map[string]interface{}{
				"key":    "attribute1",
				"value":  123,
				"action": "insert",
			},
		},
	}

	var result attributesprocessor.Config
	err := mapstructure.Decode(input, &result)
	require.NoError(t, err)
}

In the above unit test I tried adding this line:

require.Equal(t, "insert", result.Actions[0].Action)

The test failed with this error:

Error:      	Not equal: 
            	expected: string("insert")
           	actual  : attraction.Action("insert")

When tried this instead, it wouldn't compile because it can't find the attraction package (I suppose because it's internal to Otel):

require.Equal(t, attraction.Action("insert"), result.Actions[0].Action)

So I basically just used a mapstructure.Decode call to get an object from within the Otel, which is internal. I can't even initialise it on my own if I wanted. Not sure how it's possible... I can't wrap my mind around this 🤯 Anyway, I'll try to write something up to create an attributesprocessor.Config in a similar way. Thanks for the help!

@ptodev ptodev linked a pull request Mar 23, 2023 that will close this issue
@ptodev
Copy link
Contributor

ptodev commented Mar 23, 2023

@rfratto I made an example PR here using the "actions" data type: #3349
Would you mind taking a look please? If you think it looks good, I'll try the same with the other data types too.

@rfratto
Copy link
Member Author

rfratto commented Mar 23, 2023

I took a quick look at it, and yes, that approach is the idea for configuring OpenTelemetry components that are using internal packages.

@zackman0010
Copy link
Contributor

zackman0010 commented May 3, 2023

@rfratto Can this ticket be reopened for tracking due to the code being reverted with #3589? I'm currently trying to follow issues regarding features I'd need to switch our existing Static config to Flow, and this is one of them.

@rfratto rfratto reopened this May 3, 2023
@github-project-automation github-project-automation bot moved this from Done to In Progress in Grafana Agent (Public) May 3, 2023
@rfratto rfratto modified the milestones: v0.33.0, v0.34.0 May 3, 2023
@rfratto rfratto modified the milestones: v0.34.0, v0.35.0 Jun 12, 2023
@ptodev
Copy link
Contributor

ptodev commented Jun 14, 2023

This has been merged to main via #3822

@ptodev ptodev closed this as completed Jun 14, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Grafana Agent (Public) Jun 14, 2023
@github-actions 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 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
flow Related to Grafana Agent Flow frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
No open projects
4 participants