Add internal data -> OC compatibility shims to processors and exporters #667

dmitryax · 2020-03-19T22:11:49Z

Description:
In order to start migrating components from OC to internal data, all parts of the pipeline should be able to automatically convert back to OC if downstream component doesn't work with internal data structure yet

Testing: Unit tests

TODO: Support of mutating data processors will be added in a subsequent PR

tigrannajaryan · 2020-03-20T00:37:49Z

processor/cloningfanoutconnector.go

+// CreateMetricsCloningFanOutConnector is a placeholder function for now.
+// It supposed to create an old type connector or a new type connector based on type of provided metrics consumer.
+func CreateMetricsCloningFanOutConnector(baseMetricsConsumers []consumer.BaseMetricsConsumer) consumer.MetricsConsumer {
+	// TODO: CreateMetricsCloningFanOutConnector doesn't support new type of consumers


Can we make this fails loudly if we attempt to use this function until this TODO is done? Otherwise this may become a hard to catch latent race bug if we forget to implement it. panic is fine if we cannot return an error. I know we don't yet have any components that implement the new V2 interfaces, so the panic will not be actually hit. It will just be a safeguard that we will discover the problem quickly if we forget about it.

Good point. Added panic

tigrannajaryan · 2020-03-20T00:42:55Z

processor/fanoutconnector.go

+	metricsConsumersV2 := make([]consumer.MetricsConsumerV2, 0, len(mcs))
+	anyMetricsConsumersV2 := false
+	for _, mc := range mcs {
+		metricsConsumer := mc.(consumer.MetricsConsumer)


Shouldn't this be?

Suggested change

metricsConsumer := mc.(consumer.MetricsConsumer)

metricsConsumer, ok := mc.(consumer.MetricsConsumer)

if ok ...

Otherwise if mc is consumer.MetricsConsumerV2 this will panic.

Correct. Caught it when adding tests as well. Moved it under the right condition.

tigrannajaryan · 2020-03-20T00:48:09Z

processor/fanoutconnector.go

+	traceConsumersV2 := make([]consumer.TraceConsumerV2, 0, len(tcs))
+	anyTraceConsumersV2 := false
+	for _, tc := range tcs {
+		traceConsumer := tc.(consumer.TraceConsumer)


tigrannajaryan · 2020-03-20T00:50:38Z

service/builder/pipelines_builder.go

+	}
+
+	// Old type processor and a new type consumer usecase is not supported.
+	return nil, errors.New("OC Traces -> internal data format translation is not supported")


Can you add a TODO to also support this case? We now have Oc->internal translation so this should not be difficult to implement. This will allow us to mix old and new style components in the pipeline.
We can implement it in a separate PR.

Yes, makes sense. Updated

codecov-io · 2020-03-20T01:17:23Z

Codecov Report

Merging #667 into master will decrease coverage by 0.16%.
The diff coverage is 59.67%.

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
- Coverage   75.98%   75.82%   -0.17%     
==========================================
  Files         164      164              
  Lines       11614    11710      +96     
==========================================
+ Hits         8825     8879      +54     
- Misses       2365     2403      +38     
- Partials      424      428       +4

Impacted Files	Coverage Δ
processor/cloningfanoutconnector.go	`61.90% <0.00%> (-21.08%)`	⬇️
service/builder/exporters_builder.go	`64.07% <42.85%> (-4.06%)`	⬇️
service/builder/pipelines_builder.go	`75.65% <47.05%> (-11.45%)`	⬇️
service/builder/receivers_builder.go	`76.22% <75.00%> (ø)`
processor/fanoutconnector.go	`90.32% <86.95%> (-9.68%)`	⬇️
config/config.go	`99.24% <100.00%> (ø)`
exporter/factory.go	`100.00% <100.00%> (ø)`
processor/factory.go	`100.00% <100.00%> (ø)`
translator/internaldata/resource_to_oc.go	`70.14% <0.00%> (+5.97%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dc3a7d...a5b173c. Read the comment docs.

tigrannajaryan · 2020-03-24T18:07:11Z

@dmitryax the build failed.

In order to start migrating components from OC to internal data, all parts of the pipeline should be able to automatically convert back to OC if downstream component doesn't work with internal data structure

pjanotti

LGTM. Nit: the type assertions with else clauses or with returning in the ok case perhaps a better represented with type switches, and it makes the "default" case more explicit.

…lemetry#667) * Switch MinMaxSumCount to a mutex lock instead of StateLocker With multiple values being modified for each Update(), a single mutex lock and non-atomic operations ends up being faster than making each value update into an atomic operation. * Remove StateLocker implementation and comparison benchmarks * Remove field offset tests. No longer required with no atomics. Co-authored-by: Joshua MacDonald <[email protected]>

Fixes open-telemetry#657 With the changes in open-telemetry#667 and open-telemetry#669 to use a plain-old-mutex for concurrent access of Histogram and MinMaxSumCount aggregators, the StateLocker implementation is no longer used in the project. Co-authored-by: Joshua MacDonald <[email protected]>

dmitryax requested review from bogdandrutu, flands, owais, pjanotti, rghetia, songy23 and tigrannajaryan as code owners March 19, 2020 22:11

dmitryax force-pushed the feature/dmitryax/shims branch from a161f2d to a5274d1 Compare March 19, 2020 22:42

tigrannajaryan reviewed Mar 20, 2020

View reviewed changes

dmitryax force-pushed the feature/dmitryax/shims branch from a5274d1 to c3d571e Compare March 20, 2020 01:01

dmitryax force-pushed the feature/dmitryax/shims branch from c3d571e to 0dc5567 Compare March 20, 2020 01:17

tigrannajaryan approved these changes Mar 20, 2020

View reviewed changes

dmitryax force-pushed the feature/dmitryax/shims branch from 0dc5567 to 4e40990 Compare March 24, 2020 16:47

dmitryax force-pushed the feature/dmitryax/shims branch 2 times, most recently from a5b173c to abe42d4 Compare March 24, 2020 19:28

Add internal data -> OC compatibility shims to processors and exporters

67a6c08

In order to start migrating components from OC to internal data, all parts of the pipeline should be able to automatically convert back to OC if downstream component doesn't work with internal data structure

dmitryax force-pushed the feature/dmitryax/shims branch from abe42d4 to 67a6c08 Compare March 26, 2020 04:16

pjanotti approved these changes Mar 26, 2020

View reviewed changes

pjanotti merged commit 7a85f6f into open-telemetry:master Mar 26, 2020

dmitryax mentioned this pull request Apr 8, 2020

REQUEST: New membership for @dmitryax open-telemetry/community#327

Closed

6 tasks

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Update env vars in windows installer script (open-telemetry#667)

8c8eb18

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Add SpanContext (and TraceState) to Recordable (open-telemetry#667)

aeb8230

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal data -> OC compatibility shims to processors and exporters #667

Add internal data -> OC compatibility shims to processors and exporters #667

dmitryax commented Mar 19, 2020 •

edited

Loading

tigrannajaryan Mar 20, 2020

dmitryax Mar 20, 2020

tigrannajaryan Mar 20, 2020

dmitryax Mar 20, 2020

tigrannajaryan Mar 20, 2020

tigrannajaryan Mar 20, 2020

dmitryax Mar 20, 2020

codecov-io commented Mar 20, 2020 •

edited

Loading

tigrannajaryan commented Mar 24, 2020

pjanotti left a comment

	metricsConsumer := mc.(consumer.MetricsConsumer)
	metricsConsumer, ok := mc.(consumer.MetricsConsumer)
	if ok ...

Add internal data -> OC compatibility shims to processors and exporters #667

Add internal data -> OC compatibility shims to processors and exporters #667

Conversation

dmitryax commented Mar 19, 2020 • edited Loading

tigrannajaryan Mar 20, 2020

Choose a reason for hiding this comment

dmitryax Mar 20, 2020

Choose a reason for hiding this comment

tigrannajaryan Mar 20, 2020

Choose a reason for hiding this comment

dmitryax Mar 20, 2020

Choose a reason for hiding this comment

tigrannajaryan Mar 20, 2020

Choose a reason for hiding this comment

tigrannajaryan Mar 20, 2020

Choose a reason for hiding this comment

dmitryax Mar 20, 2020

Choose a reason for hiding this comment

codecov-io commented Mar 20, 2020 • edited Loading

Codecov Report

tigrannajaryan commented Mar 24, 2020

pjanotti left a comment

Choose a reason for hiding this comment

dmitryax commented Mar 19, 2020 •

edited

Loading

codecov-io commented Mar 20, 2020 •

edited

Loading