Add helpers to construct `connector.*Router`s for tests #7673

mwear · 2023-05-11T19:10:52Z

Description:
This PR adds helpers to connectortest to aid the construction of connector.*Routers for testing connectors. This was implemented based on @djaglowski's comment here, with some slight modifications. I found it more ergonomic to pass the sink into the WithTracesSink (and similar) options. You usually want a handle on the sink after creating the router. While it's possible to get the sink out of the router after the fact, it's a little cumbersome.

These helpers will be useful for open-telemetry/opentelemetry-collector-contrib#21498 and future connectors that need use of routers.

For example, here is a test with the consumer passed in:

func TestFanoutTracesWithSink(t *testing.T) {
    var sink0, sink1 consumertest.TracesSink

    tr, err := NewTracesRouterSink(
        WithTracesSink(component.NewIDWithName(component.DataTypeTraces, "0"), &sink0),
        WithTracesSink(component.NewIDWithName(component.DataTypeTraces, "1"), &sink1),
    )

    require.NoError(t, err)
    require.Equal(t, 0, sink0.SpanCount())
    require.Equal(t, 0, sink1.SpanCount())

    td := testdata.GenerateTraces(1)
    err = tr.(consumer.Traces).ConsumeTraces(context.Background(), td)

    require.NoError(t, err)
    require.Equal(t, 1, sink0.SpanCount())
    require.Equal(t, 1, sink1.SpanCount())
}

The same test having to extract the consumer out after the fact:

func TestFanoutTracesWithSink(t *testing.T) {
    traces0 := component.NewIDWithName(component.DataTypeTraces, "0")
    traces1 := component.NewIDWithName(component.DataTypeTraces, "1")

    tr, err := NewTracesRouterSink(
        WithTracesSink(traces0),
        WithTracesSink(traces1),
    )
   
    require.NoError(t, err)
   
    cons0, _ := tr.Consumer(traces0)
    sink0 := cons0.(*consumertest.TracesSink)
    cons1, _ := tr.Consumer(traces1)
    sink1 := cons1.(*consumertest.TracesSink)

    require.Equal(t, 0, sink0.SpanCount())
    require.Equal(t, 0, sink1.SpanCount())

    td := testdata.GenerateTraces(1)
    err = tr.(consumer.Traces).ConsumeTraces(context.Background(), td)

    require.NoError(t, err)
    require.Equal(t, 1, sink0.SpanCount())
    require.Equal(t, 1, sink1.SpanCount())}
}

Link to tracking Issue:
#7672

Testing:
Unit tests

Documentation:
Source code comments

codecov · 2023-05-11T19:19:19Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04 ⚠️

Comparison is base (05d57d9) 91.14% compared to head (b7c6238) 91.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7673      +/-   ##
==========================================
- Coverage   91.14%   91.10%   -0.04%     
==========================================
  Files         298      299       +1     
  Lines       14912    14942      +30     
==========================================
+ Hits        13591    13613      +22     
- Misses       1045     1051       +6     
- Partials      276      278       +2

Impacted Files	Coverage Δ
internal/fanoutconsumer/logs.go	`100.00% <ø> (ø)`
internal/fanoutconsumer/metrics.go	`100.00% <ø> (ø)`
internal/fanoutconsumer/traces.go	`100.00% <ø> (ø)`
service/internal/graph/graph.go	`98.68% <ø> (ø)`
service/internal/graph/nodes.go	`97.18% <ø> (ø)`
connector/connectortest/router.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

djaglowski

Generally looks good to me. Just a couple nits.

connector/connectortest/fanoutconsumer.go

djaglowski

LGTM.

@bogdandrutu, do you care to review this?

bogdandrutu

Same changes to all signal types.

connector/connectortest/router.go

bogdandrutu · 2023-05-12T14:42:51Z

connector/connectortest/router.go

+}
+
+// WithTracesSink adds a consumer to a connector.TracesRouter
+func WithTracesSink(id component.ID, sink *consumertest.TracesSink) TracesRouterTestOption {


Why not just WithTraces and accept a consumer.Traces

This was my suggestion. The idea was to limit the usefulness of the TracesRouter to tests only.

If we accept any consumer.Traces, then we might want to just export fanoutconsumer and let people build routers directly.

May not be a bad idea (not the whole fanoutconsumer) only the router part correct? You don't need the fanoutconsumer.NewLogs.

Is there any reason why someone would need to create a router other than for testing purposes?

I propose we expose additional functionality later, if it proves necessary.

@bogdandrutu, WDYT?

connector/connectortest/router.go

codeboten

This looks good to me, happy to merge once @bogdandrutu's confirmed his feedback has been addressed

bogdandrutu · 2023-05-31T16:16:28Z

Alternative that I have in my mind is:

Make TracesRouter a proper struct and change the factory funcs like CreateMetricsToTraces(ctx context.Context, set CreateSettings, cfg component.Config, router *TracesRouter) (Metrics, error)

type TracesRouter struct {
...
}

func (*TracesRouter) Consumer(...component.ID) (consumer.Traces, error) {}
func (*TracesRouter) PipelineIDs() []component.ID {}

Main idea is that we don't expect alternative implementation of the interface, and we also solve the inconsistency between 1 vs multiple pipelines. Another benefit is that right now we will have 2 implementations of the interface (test and non-test).

djaglowski · 2023-05-31T16:51:46Z

change the factory funcs like CreateMetricsToTraces(ctx context.Context, set CreateSettings, cfg component.Config, router *TracesRouter) (Metrics, error)

I like the idea of being more explicit about what is being provided to the factory funcs but I think this signature implies that all connectors are routers and forces all connector authors to work with this new struct. Many (maybe most) connectors should not be concerned with routing and only need a "consumer.Traces" like all other pipeline components.

The other problem is that this would be a breaking change to the connector package, which I think we should avoid even more than potential breaking changes to connectortest.

What if instead, we define a set of factory funcs specifically for routers? e.g. CreateMetricsToTracesRouter(ctx context.Context, set CreateSettings, cfg component.Config, router *TracesRouter) (Metrics, error) This exposes more surface area, but does so in a cleaner way and without breaking changes.

Another option is:

Solve the inconsistency problem by always passing in a fanout consumer, even if the consumer only emits to one pipeline.
Accept this PR or something similar (e.g. connectortest exposes a more generalized NewTracesRouter function that we are more confident will not need to change in the future)

mwear · 2023-05-31T20:02:59Z

I'm struggling to pick a clear winner between these options. I agree with @djaglowski that passing a router into the factory will be confusing for connectors that do not do routing (which is probably the majority use case). I like the option where we keep the factory functions as is, but always pass in a fanoutconsumer or the status quo. In both cases we'll need expose the necessary types for tests. I'm not completely opposed to adding additional factory functions for routers, but I think we still end up in the same situation where the connector internals will need to decide which factory function to call, likely based on the number of consumers (as it is today). If we can agree on an approach, I'd be happy to see the work through.

djaglowski · 2023-06-01T14:07:29Z

Solve the inconsistency problem by always passing in a fanout consumer, even if the consumer only emits to one pipeline.

Accept this PR or something similar (e.g. connectortest exposes a more generalized NewTracesRouter function that we are more confident will not need to change in the future)

This is my preferred approach. It's simple, non-breaking, and solves both problems. @bogdandrutu, can we move forward with this?

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>

the too few pipelines issue was resolved by open-telemetry#7840

mwear · 2023-06-16T15:07:04Z

@bogdandrutu, if you get a chance, could you take another look at this. It goes along with #7840 and solves all of the issues I ran into in open-telemetry/opentelemetry-collector-contrib#21498.

djaglowski

This should be merged in my opinion.

codeboten · 2023-06-22T21:26:35Z

/easycla

codeboten

I believe all the concerns were addressed w/ the discussion around #7840 and this PR from last week's SIG call.

@djaglowski

**Description:** This PR introduces a connector version of the routing processor. It currently supports routing based on resource attributes OTTL statements only. Context based routing will be added later in a followup PR. There are two issues regarding fanout consumers that are being addressed in the core repo. * The routing connector needs to be a consumer in multiple pipelines (the routing processor can route to a single exporter). * Will be resolved by: open-telemetry/opentelemetry-collector#7840 * We need the ability to create connector routers to facilitate testing. I had to temporarily copy the fanoutconsumer package into the routing connector codebase due to package visibility issues. * Will be resolved by: open-telemetry/opentelemetry-collector#7673 We can address these issues as the relevant PRs land on the core repo. cc: @djaglowski, @jpkrohling, @kovrus **Link to tracking Issue:** #19738 **Testing:** Unit / manual **Documentation:** The readme has been ported from the routing processor --------- Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com> Co-authored-by: Ruslan Kovalov <ruslan.kovalov@grafana.com>

mwear requested review from a team and Aneurysm9 May 11, 2023 19:10

djaglowski reviewed May 11, 2023

View reviewed changes

connector/connectortest/fanoutconsumer.go Outdated Show resolved Hide resolved

connector/connectortest/fanoutconsumer.go Outdated Show resolved Hide resolved

connector/connectortest/fanoutconsumer.go Outdated Show resolved Hide resolved

djaglowski approved these changes May 12, 2023

View reviewed changes

bogdandrutu reviewed May 12, 2023

View reviewed changes

codeboten approved these changes May 12, 2023

View reviewed changes

mwear mentioned this pull request May 12, 2023

[routingconnector] convert routing processor to a connector open-telemetry/opentelemetry-collector-contrib#21498

Merged

mwear force-pushed the connector_router_test_helpers branch from b749609 to cb6a3c5 Compare May 22, 2023 16:07

mwear mentioned this pull request Jun 6, 2023

Always pass fanout consumers to connector factory functions #7840

Merged

mwear and others added 9 commits June 14, 2023 15:46

move fanoutconsumer from service/internal to internal

93cf17d

add helpers to construct connector.*Routers for tests

155af3b

add changelog entry

a2240d1

update comments

af0928e

lint

f007bbe

Update connector/connectortest/fanoutconsumer.go

3110e13

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>

rename fanoutconsumer -> router

854165e

better names

06ca761

use shortform license

0d21adf

mwear force-pushed the connector_router_test_helpers branch from cb6a3c5 to 2a8230c Compare June 14, 2023 22:59

remove too few pipelines error

b7c6238

the too few pipelines issue was resolved by open-telemetry#7840

mwear force-pushed the connector_router_test_helpers branch from 2a8230c to b7c6238 Compare June 14, 2023 23:06

djaglowski approved these changes Jun 19, 2023

View reviewed changes

codeboten approved these changes Jun 22, 2023

View reviewed changes

codeboten merged commit fd72651 into open-telemetry:main Jun 22, 2023

github-actions bot added this to the next release milestone Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helpers to construct `connector.*Router`s for tests #7673

Add helpers to construct `connector.*Router`s for tests #7673

mwear commented May 11, 2023

codecov bot commented May 11, 2023 •

edited

Loading

djaglowski left a comment

djaglowski left a comment

bogdandrutu left a comment

bogdandrutu May 12, 2023

djaglowski May 12, 2023

bogdandrutu May 12, 2023

mwear May 12, 2023

djaglowski May 15, 2023

djaglowski May 22, 2023

codeboten left a comment

bogdandrutu commented May 31, 2023

djaglowski commented May 31, 2023

mwear commented May 31, 2023 •

edited

Loading

djaglowski commented Jun 1, 2023

mwear commented Jun 16, 2023

djaglowski left a comment

codeboten commented Jun 22, 2023

codeboten left a comment

Add helpers to construct connector.*Routers for tests #7673

Add helpers to construct connector.*Routers for tests #7673

Conversation

mwear commented May 11, 2023

codecov bot commented May 11, 2023 • edited Loading

Codecov Report

djaglowski left a comment

Choose a reason for hiding this comment

djaglowski left a comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu May 12, 2023

Choose a reason for hiding this comment

djaglowski May 12, 2023

Choose a reason for hiding this comment

bogdandrutu May 12, 2023

Choose a reason for hiding this comment

mwear May 12, 2023

Choose a reason for hiding this comment

djaglowski May 15, 2023

Choose a reason for hiding this comment

djaglowski May 22, 2023

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

bogdandrutu commented May 31, 2023

djaglowski commented May 31, 2023

mwear commented May 31, 2023 • edited Loading

djaglowski commented Jun 1, 2023

mwear commented Jun 16, 2023

djaglowski left a comment

Choose a reason for hiding this comment

codeboten commented Jun 22, 2023

codeboten left a comment

Choose a reason for hiding this comment

Add helpers to construct `connector.*Router`s for tests #7673

Add helpers to construct `connector.*Router`s for tests #7673

codecov bot commented May 11, 2023 •

edited

Loading

mwear commented May 31, 2023 •

edited

Loading