Port spanmetricsprocessor to spanmetricsconnector

[djaglowski]

As noted in the spanmetricsprocessor readme, it has been intended for quite some time that the component will be migrated to become a connector. Now that the connectors framework is mostly in place, I worked through a preliminary implementation of the component as a connector, starting from a copy/paste of the processor. Overall, I think the implementation and user facing config are both much simpler.

For now I'm looking for feedback on any part of the implementation. I believe we need to wait for the next update from the core collector repo before this component will be manually testable.

[runforesight]

Foresight Summary

    
Major Impacts

build-and-test-windows duration(4 seconds) has decreased 40 minutes 13 seconds compared to main branch avg(40 minutes 17 seconds).

View More Details

⭕  build-and-test-windows workflow has finished in 4 seconds (40 minutes 22 seconds less than main branch avg.) and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
windows-unittest-matrix	-     🔗	N/A	See Details
windows-unittest	-     🔗	N/A	See Details

---

✅  telemetrygen workflow has finished in 2 minutes 15 seconds and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
build-dev	-     🔗	N/A	See Details
publish-latest	-     🔗	N/A	See Details
publish-stable	-     🔗	N/A	See Details

---

❌  changelog workflow has finished in 2 minutes 18 seconds and finished at 7th Feb, 2023. 1 job failed.

---

Job	Failed Steps	Tests
changelog	Ensure ./.chloggen/*.yaml addition(s)     🔗	N/A	See Details

---

✅  check-links workflow has finished in 3 minutes 1 second (⚠️ 50 seconds more than main branch avg.) and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
changed files	-     🔗	N/A	See Details
check-links	-     🔗	N/A	See Details

---

✅  prometheus-compliance-tests workflow has finished in 12 minutes 40 seconds (⚠️ 4 minutes 36 seconds more than main branch avg.) and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
prometheus-compliance-tests	-     🔗	✅ 21  ❌ 0  ⏭ 0    🔗	See Details

---

✅  build-and-test workflow has finished in 48 minutes 22 seconds (7 minutes 11 seconds less than main branch avg.) and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
correctness-metrics	-     🔗	✅ 2  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, internal)	-     🔗	✅ 561  ❌ 0  ⏭ 0    🔗	See Details
correctness-traces	-     🔗	✅ 17  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, processor)	-     🔗	✅ 1472  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, extension)	-     🔗	✅ 546  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, processor)	-     🔗	✅ 1472  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, internal)	-     🔗	✅ 561  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, extension)	-     🔗	✅ 546  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, receiver-0)	-     🔗	✅ 2578  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, receiver-0)	-     🔗	✅ 2578  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, exporter)	-     🔗	✅ 2444  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, exporter)	-     🔗	✅ 2444  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, receiver-1)	-     🔗	✅ 1958  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.19, other)	-     🔗	✅ 4729  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, receiver-1)	-     🔗	✅ 1958  ❌ 0  ⏭ 0    🔗	See Details
unittest-matrix (1.18, other)	-     🔗	✅ 4729  ❌ 0  ⏭ 0    🔗	See Details
integration-tests	-     🔗	✅ 55  ❌ 0  ⏭ 0    🔗	See Details
setup-environment	-     🔗	N/A	See Details
check-codeowners	-     🔗	N/A	See Details
lint-matrix (receiver-0)	-     🔗	N/A	See Details
lint-matrix (receiver-1)	-     🔗	N/A	See Details
lint-matrix (processor)	-     🔗	N/A	See Details
lint-matrix (exporter)	-     🔗	N/A	See Details
lint-matrix (extension)	-     🔗	N/A	See Details
lint-matrix (internal)	-     🔗	N/A	See Details
check-collector-module-version	-     🔗	N/A	See Details
lint-matrix (other)	-     🔗	N/A	See Details
checks	-     🔗	N/A	See Details
build-examples	-     🔗	N/A	See Details
unittest (1.19)	-     🔗	N/A	See Details
unittest (1.18)	-     🔗	N/A	See Details
lint	-     🔗	N/A	See Details
cross-compile (darwin, amd64)	-     🔗	N/A	See Details
cross-compile (darwin, arm64)	-     🔗	N/A	See Details
cross-compile (linux, 386)	-     🔗	N/A	See Details
cross-compile (linux, amd64)	-     🔗	N/A	See Details
cross-compile (linux, arm)	-     🔗	N/A	See Details
cross-compile (linux, arm64)	-     🔗	N/A	See Details
cross-compile (linux, ppc64le)	-     🔗	N/A	See Details
cross-compile (windows, 386)	-     🔗	N/A	See Details
cross-compile (windows, amd64)	-     🔗	N/A	See Details
build-package (deb)	-     🔗	N/A	See Details
build-package (rpm)	-     🔗	N/A	See Details
windows-msi	-     🔗	N/A	See Details
publish-check	-     🔗	N/A	See Details
publish-stable	-     🔗	N/A	See Details
publish-dev	-     🔗	N/A	See Details

---

✅  e2e-tests workflow has finished in 15 minutes 35 seconds and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
kubernetes-test	-     🔗	N/A	See Details

---

✅  load-tests workflow has finished in 20 minutes 14 seconds (⚠️ 4 minutes 50 seconds more than main branch avg.) and finished at 7th Feb, 2023.

---

Job	Failed Steps	Tests
loadtest (TestMetric10kDPS|TestMetricsFromFile)	-     🔗	✅ 6  ❌ 0  ⏭ 0    🔗	See Details
loadtest (TestTraceAttributesProcessor)	-     🔗	✅ 3  ❌ 0  ⏭ 0    🔗	See Details
loadtest (TestIdleMode)	-     🔗	✅ 1  ❌ 0  ⏭ 0    🔗	See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs)	-     🔗	✅ 8  ❌ 0  ⏭ 0    🔗	See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS)	-     🔗	✅ 12  ❌ 0  ⏭ 0    🔗	See Details
loadtest (TestBallastMemory|TestLog10kDPS)	-     🔗	✅ 19  ❌ 0  ⏭ 0    🔗	See Details
setup-environment	-     🔗	N/A	See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs)	-     🔗	✅ 10  ❌ 0  ⏭ 0    🔗	See Details

---

🔎 See details on Foresight

^{*You can configure Foresight comments in your organization settings page.}

[djaglowski]

I'm seeing a noticeable performance gain here too:

pkg: github.com/open-telemetry/opentelemetry-collector-contrib/connector/spanmetricsconnector
BenchmarkConnectorConsumeTraces-10    	  483807	      2420 ns/op	    1609 B/op	      39 allocs/op

pkg: github.com/open-telemetry/opentelemetry-collector-contrib/processor/spanmetricsprocessor
BenchmarkProcessorConsumeTraces-10    	   53926	     19852 ns/op	   10555 B/op	     127 allocs/op

[jpkrohling]

cc @kovrus (and @mapno?)

[djaglowski]

I wrote up a lengthy response to a comment about having the connector pass traces through it like the processor does. The comment was deleted but I'm posting it here for future reference because I think it lays out a good case against it and I expect this idea will come up again.

---

Whether or not the connector should forward the data is a good question - one I expect we'll see a lot. I've gone back and forth on this myself, but landed on the side of not forwarding.

I see this connector as representative of a class of connectors whose primary purpose is to generate new data. I'll call them "generative" connectors for now. The count connector is another in this class. Just so we have another example, suppose a logalerts connector generates an "alert" (i.e. a log with semantic guarantees) when it sees a FATAL log record.

Note that there is a meaningful distinction between the generated data and the consumed data. Unfortunately, in many cases these data sets would be mixed together as we emit both to the same pipeline. This problem is not apparent when looking only at spanmetrics, but it is a clear issue for logalerts and an issue with count as well.

There are lots of good reasons someone would want to keep the data sets separate and I think we should prefer a solution that naturally avoids mixing them.

Fortunately, we have a simple solution - don't embed forwarding into generative connectors. In my opinion, this is better in terms of usability as well as development & maintainability. Ultimately, it is not difficult to explicitly use forward.

# spanmetrics does forward traces
pipelines:
  traces:
    receivers: [foo]
    exporters: [spanmetrics]
  traces/out:
    receivers: [spanmetrics]
    exporters: [jaeger]
  metrics:
    receivers: [spanmetrics]
    exporters: [prometheus]

# spanmetrics does not forwards traces
pipelines:
  traces:
    receivers: [foo]
    exporters: [forward, spanmetrics]
  traces/out:
    receivers: [foward]
    exporters: [jaeger]
  metrics:
    receivers: [spanmetrics]
    exporters: [prometheus]

The alternative would seem to be that connector configurations allow users to declare where to send either the generated data or the consumed data. When I tried to work through the details of how exactly this would work, I found that it results in a very inconsistent experience across generative connectors. (Also just to clarify, I'm in favor of referencing pipelines in a connector config, when it's natural. e.g. routing).

Here's a specific usability problem that comes with the pass-through capability.

A nice feature about connectors is that you can use their capabilities a la carte. Meaning, a user can connect a traces pipeline to a metrics pipeline, and the connector will only perform the function that consumes traces and emits metrics.

# opt out of forwarding traces
pipelines:
  traces:
    receivers: [foo]
    exporters: [spanmetrics]
# Uncomment to forward traces
#  traces/out:
#   receivers: [spanmetrics]
#   exporters: [jaeger]
  metrics:
    receivers: [spanmetrics]
    exporters: [prometheus]

Unfortunately this means that this is also a valid config.

# 'spanmetrics' behaves exactly as 'forward'
pipelines:
  traces:
    receivers: [foo]
    exporters: [spanmetrics]
  traces/out:
    receivers: [spanmetrics]
    exporters: [jaeger]

The more I look at the pass-through the more reason I find not to have it in these generative connectors.

[djaglowski]

Closing in favor of #18535