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

Fix bug where MutatesData would not correctly propogate through connectors #9053

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented Dec 7, 2023

This fixes two closely related problems.

  1. While fanoutconsumers do not themselves mutate data, they should expose whether or not they are handing data off to consumers which may do so. Otherwise, the service cannot correctly determine how to fan out after a receiver. e.g. a receiver shared between two pipelines, one of which contains an exporter or connector which mutates data.
  2. Connectors can themselves mutate data but we were not taking this into account when building the graph.

@djaglowski djaglowski force-pushed the connectors-propogate-mutates-data branch from a735d8b to 3ad326f Compare December 7, 2023 19:52
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dc28ec1) 91.57% compared to head (6a03bda) 91.57%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9053   +/-   ##
=======================================
  Coverage   91.57%   91.57%           
=======================================
  Files         316      316           
  Lines       17147    17161   +14     
=======================================
+ Hits        15702    15716   +14     
  Misses       1150     1150           
  Partials      295      295           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djaglowski djaglowski marked this pull request as ready for review December 7, 2023 20:05
@djaglowski djaglowski requested a review from a team as a code owner December 7, 2023 20:05
service/internal/graph/graph.go Show resolved Hide resolved
service/internal/graph/nodes.go Outdated Show resolved Hide resolved
@djaglowski djaglowski force-pushed the connectors-propogate-mutates-data branch from 3ad326f to 41274a2 Compare December 7, 2023 21:48
@djaglowski djaglowski force-pushed the connectors-propogate-mutates-data branch from 41274a2 to 6a03bda Compare December 7, 2023 21:48
@@ -42,7 +42,7 @@ type metricsConsumer struct {
}

func (msc *metricsConsumer) Capabilities() consumer.Capabilities {
return consumer.Capabilities{MutatesData: false}
return consumer.Capabilities{MutatesData: len(msc.mutable) > 0}
Copy link
Member

@dmitryax dmitryax Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a readonly receiver in the fan-out, it's not mutating. The mutable data will be copied

Suggested change
return consumer.Capabilities{MutatesData: len(msc.mutable) > 0}
return consumer.Capabilities{MutatesData: len(msc.mutable) > 0 && len(msc. readonly) == 0}

@bogdandrutu bogdandrutu merged commit 7c58e71 into open-telemetry:main Dec 9, 2023
32 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 9, 2023
@djaglowski djaglowski deleted the connectors-propogate-mutates-data branch December 11, 2023 15:39
dmitryax pushed a commit that referenced this pull request Dec 11, 2023
Follow up to
#9053.

@dmitryax pointed out
[here](#9053 (comment))
that the fanout consumer will pass original data to a non-mutating
consumer if any is available. This PR incorporates that point and
updates test expectations accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants