Add an `otelcol.connector.spanlogs` flow component #4713

ptodev · 2023-08-04T22:33:16Z

PR Description

This is the flow mode equivalent to static mode's automatic_logging processor.

Which issue(s) this PR fixes

Fixes #4261

Notes to the Reviewer

Are we sure otelcol.processor.spanlogs is a good name for this?
Initially I wanted to reuse as much code as possible between static and flow modes. Later I gave up on this. I don't think it's very important.
The usage of []any in the original code made it look a bit weird. Since this is a purely otel component which doesn't interact with Loki, I decided to use Otel's pcommon.Map. This is the data structure which span attributes are normally stored in. However, this data structure is inefficient when reading and writing to it, since it's actually an array and we need to traverse it on every insertion. Maybe I should switch to a Go map?

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

clayton-cornell

No changes for docs.

mattdurham · 2023-08-07T14:27:47Z

component/otelcol/processor/spanlogs/consumer.go

+			spanLen := ss.Spans().Len()
+
+			lastTraceID := ""
+			for k := 0; k < spanLen; k++ {


Can we do something about this level of indentation, even if its moving to a few different functions?

Yes, I refactored it now, PTAL.

mattdurham · 2023-08-07T14:29:30Z

component/otelcol/processor/spanlogs/consumer.go

+					c.spanKeyVals(keyValues, span)
+					c.processKeyVals(keyValues, rs.Resource(), svc)
+
+					newLogRecord, err := c.createLogRecord(typeSpan, traceID, keyValues)


Is the only difference in these code blocks the first arg to createLogRecord? If so can we make it a function where you pass in the type?

I refactored it now, PTAL.

component/otelcol/processor/spanlogs/consumer.go

mattdurham · 2023-08-07T14:31:04Z

component/otelcol/processor/spanlogs/consumer.go

+func (c *consumer) processKeyVals(output pcommon.Map, resource pcommon.Resource, svc string) {
+	rsAtts := resource.Attributes()
+
+	// name


I dont understand this comment

Haha, this is copy pasted from automaticloggingprocessor. You're right that it's unclear. I'll replace it with this:

// Add an attribute with the service name

clayton-cornell

Some tweaks to the docs

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

ptodev · 2023-08-24T15:44:03Z

Hi @mattdurham - I updated the PR again, and I think it's ready for review now (again 😅 ). This is what I last changed:

I renamed the component from "processor" to "connector"
I tested this locally and it works - I see logs on Grafana Cloud, and I see labels for the logs too.
I added the component to all.go
Added an entry to the changelog.

mattdurham · 2023-08-25T13:22:45Z

component/otelcol/connector/spanlogs/consumer.go

+}
+
+func convertOtelMapToSlice(m pcommon.Map) []any {
+	res := make([]any, 0, m.Len())


Should m.len() be m.len() * 2? Since we are flattening the map?

This is true! Well spotted, thank you, I'll change it.

mattdurham · 2023-08-25T13:26:49Z

component/otelcol/connector/spanlogs/consumer.go

+		att, ok := rsAtts.Get(name)
+		if ok {
+			// name/key val pairs
+			val := output.PutEmpty(name)


Not sure what we are doing here. So if we find a value in rsAtts, we put an empty value into the output and then copy the empty value to att?

Yeah. It does look weird. The issue is that pcommon.Map doesn't have a function to insert a Value and a Key at the same time, like a PutValue(k string, v Value). These kinds of functions only insert specific types like PutInt(k string, v int64).

I want to insert a Value so that I preserve the original type, and this is the only way to do it as far as I can tell.

In hindsight, I should have just used a map[string]pcommon.Value instead of a pcommon.Map. It would also have been more efficient, since a pcommon.Map is actually just an slice. Inserting to it is slow, because it has to traverse the whole slice to see if the element is already in the map.

I'll add a TODO in the code for this, but for now I hope it's fast enough that a pcommon.Map will do.

mattdurham · 2023-08-25T13:28:16Z

component/otelcol/connector/spanlogs/spanlogs.go

+// New creates a new otelcol.exporter.spanlogs component.
+func New(o component.Options, c Arguments) (*Component, error) {
+	if c.Output.Traces != nil || c.Output.Metrics != nil {
+		level.Warn(o.Logger).Log("msg", "non-log output detected; this component only works for log outputs and trace inputs")


Is this true? Why is Metrics checked if logs are the only output?

c.Output is the output of a otelcol.exporter.spanlogs component. It can only output logs, so if someone configures it to output metrics or traces, it'll complain via this error message.

Unfortunately, due to the current implementation of consumer.go, we always pass metrics, logs, and traces to each otel component regardless of whether the component supports them. I'll raise a separate issue to find a nicer long term solution to this for components which don't support all 3 telemetry signals.

mattdurham

A few comments but looking good.

mattdurham

Lgtm

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>

Fix warning message

Add otelcol.connector.spanlogs to all.go. Add otelcol.connector.spanlogs to changelog.

ptodev requested review from a team and clayton-cornell as code owners August 4, 2023 22:33

ptodev linked an issue Aug 4, 2023 that may be closed by this pull request

Flow: Alternative to Static mode's tracing automatic_logging processor #4261

Closed

clayton-cornell reviewed Aug 4, 2023

View reviewed changes

mattdurham reviewed Aug 7, 2023

View reviewed changes

component/otelcol/processor/spanlogs/consumer.go Outdated Show resolved Hide resolved

mattdurham reviewed Aug 7, 2023

View reviewed changes

clayton-cornell reviewed Aug 8, 2023

View reviewed changes

ptodev force-pushed the 4261-flow-alternative-to-static-modes-tracing-automatic_logging-processor branch from 9631895 to af18bf3 Compare August 15, 2023 09:55

ptodev requested review from mattdurham and clayton-cornell August 15, 2023 10:46

ptodev force-pushed the 4261-flow-alternative-to-static-modes-tracing-automatic_logging-processor branch 2 times, most recently from 23613eb to b96081a Compare August 21, 2023 10:40

ptodev force-pushed the 4261-flow-alternative-to-static-modes-tracing-automatic_logging-processor branch from b96081a to 79b2864 Compare August 24, 2023 15:39

ptodev changed the title ~~Add an otelcol.processor.spanlogs flow component~~ Add an otelcol.connector.spanlogs flow component Aug 24, 2023

mattdurham reviewed Aug 25, 2023

View reviewed changes

ptodev force-pushed the 4261-flow-alternative-to-static-modes-tracing-automatic_logging-processor branch from 91a8250 to 2bd53d2 Compare August 25, 2023 14:07

mattdurham approved these changes Aug 25, 2023

View reviewed changes

ptodev and others added 7 commits August 25, 2023 15:37

Add an otelcol.processor.spanlogs flow component

57ab35b

Use pcommon.NewMap and add more tests

9f5d279

Improve docs

2f66e22

Update changelog

bb9b4eb

Remove empty test file

4f479f9

Reduce repetition in the code

6e10854

Apply docs suggestions from code review

d3b8764

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>

ptodev added 5 commits August 25, 2023 15:37

Return error on consumer init.

13d037a

Fix warning message

Check for error

da7072f

Rename otelcol.processor.spanlogs to "otelcol.connector.spanlogs".

7dc5c95

Add otelcol.connector.spanlogs to all.go. Add otelcol.connector.spanlogs to changelog.

PR review improvements.

6efbfb4

Fix build after rebase with main branch

051903e

ptodev force-pushed the 4261-flow-alternative-to-static-modes-tracing-automatic_logging-processor branch from a47454a to 051903e Compare August 25, 2023 14:37

Update changelog

357ae29

ptodev merged commit cc3e80d into main Aug 25, 2023

ptodev deleted the 4261-flow-alternative-to-static-modes-tracing-automatic_logging-processor branch August 25, 2023 15:00

clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Aug 31, 2023

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

Add an `otelcol.connector.spanlogs` flow component #4713

Add an `otelcol.connector.spanlogs` flow component #4713

ptodev commented Aug 4, 2023

clayton-cornell left a comment

mattdurham Aug 7, 2023 •

edited

Loading

ptodev Aug 15, 2023

mattdurham Aug 7, 2023

ptodev Aug 15, 2023 •

edited

Loading

mattdurham Aug 7, 2023

ptodev Aug 8, 2023

clayton-cornell left a comment

ptodev commented Aug 24, 2023 •

edited

Loading

mattdurham Aug 25, 2023

ptodev Aug 25, 2023

mattdurham Aug 25, 2023

ptodev Aug 25, 2023

mattdurham Aug 25, 2023

ptodev Aug 25, 2023

mattdurham left a comment

mattdurham left a comment

Add an otelcol.connector.spanlogs flow component #4713

Add an otelcol.connector.spanlogs flow component #4713

Conversation

ptodev commented Aug 4, 2023

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

clayton-cornell left a comment

Choose a reason for hiding this comment

mattdurham Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptodev Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clayton-cornell left a comment

Choose a reason for hiding this comment

ptodev commented Aug 24, 2023 • edited Loading

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

mattdurham left a comment

Choose a reason for hiding this comment

mattdurham left a comment

Choose a reason for hiding this comment

Add an `otelcol.connector.spanlogs` flow component #4713

Add an `otelcol.connector.spanlogs` flow component #4713

mattdurham Aug 7, 2023 •

edited

Loading

ptodev Aug 15, 2023 •

edited

Loading

ptodev commented Aug 24, 2023 •

edited

Loading