[pkg/stanza] add buildInfo for operator(#21794) #26664

yutingcaicyt · 2023-09-13T12:03:41Z

Description:
For adding the telemetry for filelog receiver, the related info (CreateSettings) needs to be delivered from factory to operator. So I add a BuildInfoInternal struct to do this first. And then the operators have the ability to add the metrics themselves.

Link to tracking Issue: #21794

djaglowski

Thanks for putting this together @yutingcaicyt.

I think this demonstrates what the plumbing may look like, which is helpful. I've left a few comments about this.

However, before we move forward with anything, I'd really like to see what it looks like to actually use the new parameters that we're passing through. This will help to validate what we actually need to pass through and also to inform how we should manage the addition complexity.

djaglowski · 2023-09-13T14:41:20Z

pkg/stanza/operator/config.go

@@ -16,6 +17,11 @@ type Config struct {
 	Builder
 }

+type BuildInfoInternal struct {
+	Logger         *zap.SugaredLogger
+	CreateSettings *rcvr.CreateSettings


I think in the end we cannot directly use receiver.CreateSettings because there are legitimate use cases for this codebase outside of receivers.

The struct contains several fields. Do you know which of these we actually need?

type CreateSettings struct { // ID returns the ID of the component that will be created. ID component.ID component.TelemetrySettings // BuildInfo can be used by components for informational purposes. BuildInfo component.BuildInfo }

From my understanding, we need "ID" and "component.TelemetrySettings" to add the telemetry into the operator.

djaglowski · 2023-09-13T14:42:24Z

pkg/stanza/operator/config.go

@@ -16,6 +17,11 @@ type Config struct {
 	Builder
 }

+type BuildInfoInternal struct {


Depending on how many individual parts of CreateSettings we need, I may be preferable to pass in each parameter separately, rather than have a wrapper like this.

I agree to only pass the parts of CreateSettings that we need, but I think a wrapper is also needed. I raised some questions in the comment below.

djaglowski · 2023-09-13T14:44:05Z

pkg/stanza/operator/input/file/config.go

-func (c Config) Build(logger *zap.SugaredLogger) (operator.Operator, error) {
-	inputOperator, err := c.InputConfig.Build(logger)
+func (c Config) Build(buildInfo *operator.BuildInfoInternal) (operator.Operator, error) {
+	inputOperator, err := c.InputConfig.Build(buildInfo.Logger)


Many operators rely on these composable helper structs. It's hard to say without seeing how we are using the CreateSettings, but I suspect we should pass it through and have the shared structs manage as much of the additional complexity as possible.

In my opinion, shared structs do introduce some additional complexity, but they can also facilitate iteration or refactoring. Take the example of adding telemetry, we may need another temporary parameter like "useOtel" If we have the struct, the parameter can easily pass it through without modifying all implementations of the "Builder" interface. Or there will be other parameters that need to be passed through for some new features in the future. Without the struct, all these changes would be very difficult. How do you think about these situations?

I can appreciate the utility of what you are suggesting but I think there is a tradeoff that we have additional complexity which may never be needed. Generally in this repo we strongly prefer to include only what is necessary, even though it occasionally means we need to make a change later.

So you mean that we can change the interface like this?

func (c Config) Build(logger *zap.SugaredLogger, ID component.ID, settings component.TelemetrySettings, telemetryUseOtel bool) (operator.Operator, error)

Does this really reduce much complexity compared to using struct?

So you mean that we can change the interface like this?

func (c Config) Build(logger *zap.SugaredLogger, ID component.ID, settings component.TelemetrySettings, telemetryUseOtel bool) (operator.Operator, error)

If in the future we have a need for additional parameters, they will either be required or optional. If they are required, this will be a breaking change anyways. If they are optional (or have reasonable defaults), then there are other ways to handle this beside breaking the signature. We can add variadic options or alternate signatures.

Does this really reduce much complexity compared to using struct?

Why do functions allow multiple parameters? Obviously there are tradeoffs regarding complexity.

In any case, I will continue to push for minimal complexity in the current implementation. To that end, I think we are two steps ahead of ourselves because we haven't even demonstrated how any of the proposed parameters will actually be used.

Yeah, let's first determine what parameters need to be passed. For creating the telemetry, the "component.TelemetrySettings" is needed to get the "metricsLevel" and "MeterProvider". The "ID" of the component is also needed to set the attribute of the metric. So I think "component.TelemetrySettings" and "ID" are required parameters. And I noticed that the internal metric of the collector will use this "UseOtelForInternalMetricsfeatureGate" to determine whether to use otel, if we follow the rule, a parameter "useOtel bool" is optional. In conclusion, "component.TelemetrySettings" and "ID" are required, "useOtel" is opional. WDYT?

@djaglowski

Please update the PR to demonstrate usage of the parameters.

yutingcaicyt · 2023-09-26T02:51:33Z

I added some parameters and explaination to the function.

yutingcaicyt · 2023-10-08T03:02:06Z

@djaglowski

djaglowski

I've mentioned all of the following elsewhere but to reiterate my requested changes:

Demonstrate actual usage of the information which you propose to pass through. Concrete usage will validate that we actually need these and inform the design process.
Pass the necessary info through to helper structs so that we can centralize management. e.g. InputConfig.Build.
BuildInfoInternal struct is likely unnecessary. We can reassess when items 1 and 2 are completed.

yutingcaicyt · 2023-10-10T07:53:59Z

I understand what you mean, but every change to the interface requires a huge amount of work, so I want to wait until we confirm the parameters passed before making unified changes.
I have added a metric example, which includes the parameters used. In general, I think CreateSettings and telemetryUseOtel are needed. All properties in createSettings may be used by metric, and telemetryUseOtel is used to control otel or openCensus to enable.
So I think only the two parameters need to be passed through.

djaglowski · 2023-10-11T17:18:07Z

pkg/stanza/fileconsumer/metrics.go

+type fileConsumerTelemetry struct {
+	level    configtelemetry.Level
+	detailed bool
+	useOtel  bool


@open-telemetry/collector-approvers, do we expect to support a useOtel throughout the collector? Would it be reasonable to assume we're using OTel in new instrumentation at this point?

Do we need to discuss this here? I feel that whether to use otel should be unified throughout the project.

I'd like a decision. I'll bring it up at Wednesday's SIG

Discussed this in SIG today. The consensus from core maintainers is to support only OpenCensus metrics for now. We plan to provide a bridge once open-telemetry/opentelemetry-collector#7454 is complete. Therefore, let's remove the flag.

github-actions · 2023-11-02T05:19:25Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2023-11-16T05:19:46Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

yutingcaicyt · 2024-01-23T06:05:53Z

I'm sorry that I haven't dealt with this PR for a long time due to some personal issues. To confirm that the changes are valid, I want to know whether opencensus is still used by default.

djaglowski · 2024-01-23T14:23:02Z

I'm sorry that I haven't dealt with this PR for a long time due to some personal issues. To confirm that the changes are valid, I want to know whether opencensus is still used by default.

@codeboten, could you advise here?

yutingcaicyt requested review from djaglowski and atoulme as code owners September 13, 2023 12:03

yutingcaicyt requested a review from a team September 13, 2023 12:03

github-actions bot assigned djaglowski Sep 13, 2023

github-actions bot added pkg/stanza processor/logstransform Logs Transform processor receiver/otlpjsonfile labels Sep 13, 2023

github-actions bot requested a review from dehaansa September 13, 2023 12:04

yutingcaicyt force-pushed the feature/addlogdelaymetric branch from 292484c to 9f046c4 Compare September 13, 2023 12:22

djaglowski reviewed Sep 13, 2023

View reviewed changes

yutingcaicyt force-pushed the feature/addlogdelaymetric branch from 9f046c4 to a34ba3e Compare September 26, 2023 02:46

djaglowski requested changes Oct 9, 2023

View reviewed changes

yutingcaicyt force-pushed the feature/addlogdelaymetric branch from a34ba3e to aa57ee7 Compare October 10, 2023 07:38

[pkg/stanza] add buildInfo for operator(open-telemetry#21794)

53fa50f

yutingcaicyt force-pushed the feature/addlogdelaymetric branch from aa57ee7 to 53fa50f Compare October 10, 2023 07:48

djaglowski reviewed Oct 11, 2023

View reviewed changes

github-actions bot added the Stale label Nov 2, 2023

github-actions bot closed this Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pkg/stanza] add buildInfo for operator(#21794) #26664

[pkg/stanza] add buildInfo for operator(#21794) #26664

yutingcaicyt commented Sep 13, 2023 •

edited by djaglowski

Loading

djaglowski left a comment

djaglowski Sep 13, 2023

yutingcaicyt Sep 14, 2023

djaglowski Sep 13, 2023

yutingcaicyt Sep 14, 2023

djaglowski Sep 13, 2023

yutingcaicyt Sep 14, 2023

djaglowski Sep 14, 2023

yutingcaicyt Sep 15, 2023 •

edited

Loading

djaglowski Sep 15, 2023

yutingcaicyt Sep 18, 2023

yutingcaicyt Sep 21, 2023

djaglowski Sep 21, 2023

yutingcaicyt commented Sep 26, 2023

yutingcaicyt commented Oct 8, 2023

djaglowski left a comment

yutingcaicyt commented Oct 10, 2023

djaglowski Oct 11, 2023

yutingcaicyt Oct 16, 2023

djaglowski Oct 16, 2023

djaglowski Oct 18, 2023

github-actions bot commented Nov 2, 2023

github-actions bot commented Nov 16, 2023

yutingcaicyt commented Jan 23, 2024

djaglowski commented Jan 23, 2024

[pkg/stanza] add buildInfo for operator(#21794) #26664

[pkg/stanza] add buildInfo for operator(#21794) #26664

Conversation

yutingcaicyt commented Sep 13, 2023 • edited by djaglowski Loading

djaglowski left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutingcaicyt Sep 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutingcaicyt commented Sep 26, 2023

yutingcaicyt commented Oct 8, 2023

djaglowski left a comment

Choose a reason for hiding this comment

yutingcaicyt commented Oct 10, 2023

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

github-actions bot commented Nov 2, 2023

github-actions bot commented Nov 16, 2023

yutingcaicyt commented Jan 23, 2024

djaglowski commented Jan 23, 2024

yutingcaicyt commented Sep 13, 2023 •

edited by djaglowski

Loading

yutingcaicyt Sep 15, 2023 •

edited

Loading