search

open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.43k stars 1.46k forks source link

Decouple Service from components #4605

Closed bogdandrutu closed 2 years ago

bogdandrutu commented 2 years ago

Right now the Service (configuration, logic) is very connected with the rest of components. There are use-cases where we need to allow users to consume the otel components in a different "Service" or a different way to define the service configuration. To achieve this we should move the config.Config and config.Service under the service package and allow components to not depend on anything service related.

alolita commented 2 years ago

Hi @bogdandrutu what is the reasoning behind these changes? Are you building a new service package? Is there a design document you can share with the rest of us? It would be good for maintainers to also do design docs for changes which other maintainers can also have a chance to review and provide feedback on.

alolita commented 2 years ago

Also, I thought we had a project policy of contributors from one org having reviews for their changes from approvers from other orgs (not all from the same org) for all components on OTEL - Collector included. Has this policy changed? Just wondering.

cc: @open-telemetry/governance-committee

dyladan commented 2 years ago

Also, I thought we had a project policy of contributors from one org having reviews for their changes from approvers from other orgs (not all from the same org) for all components on OTEL - Collector included. Has this policy changed? Just wondering.

cc: @open-telemetry/governance-committee

I think that policy only applied to oteps/specification. For many SIGs it's not viable to enforce such a rule with the limited number of active approvers.

bogdandrutu commented 2 years ago

@alolita here are the answers:

what is the reasoning behind these changes?

The reason as explained in the issue is to decouple the service definition from the components (e.g. otlp receiver). As it is logical, the service uses components not the other way around.

Are you building a new service package?

No. But we want to not limit ourself in the future.

Is there a design document you can share with the rest of us?

As the issue says this needs only one struct move, so it is a pretty simple change.

jpkrohling commented 2 years ago

There are use-cases where we need to allow users to consume the otel components in a different "Service" or a different way to define the service configuration.

I'm not opposed to this change, but could you give a concrete example of the above? It would certainly help make the case for this change.

bogdandrutu commented 2 years ago

@jpkrohling internally @tigrannajaryan was looking to build an internal service based on these components, he can tell you more about the status of that.

Also, it does not cost us too much (almost nothing) to keep this door open.

Aneurysm9 commented 2 years ago

I would like to know more about this change as well. Without an understanding of what the goal is and the use cases for it, it is hard to evaluate whether the change is good or correct.

internally @tigrannajaryan was looking to build an internal service based on these components

I think this reinforces the need to have review and approval from members representing multiple organizations. This change seems to have been made by Splunk, to satisfy an unstated Splunk-internal objective, and approved only by Splunk employees. This concerns me and would not be permitted in any other OTel SIG I have been involved in.

I think that policy only applied to oteps/specification. For many SIGs it's not viable to enforce such a rule with the limited number of active approvers.

This policy also applies within the Go SIG. There are almost as many approvers on the collector repo (5 approvers, all active, 2 maintainers) as the Go repo (6 approvers, fewer active, 3 maintainers). I see no reason why it should not be applicable here as well.

alolita commented 2 years ago

@bogdandrutu @tigrannajaryan there are approvers from other orgs as well as many active contributors on the Collector. As you know, the Collector is used and maintained by the larger observability community and stakeholders. It would be great to communicate and share design change proposals before taking unilateral decisions from a single org for internal dependencies. Imo, the project cannot serve as a playground for any individual vendor. It should be a collaborative effort to build an industry standard and components such as the Collector and SDKs based on this standard. Please share any design docs with all of us.

MovieStoreGuy commented 2 years ago

As someone that is effectively a customer of the project, and a highly active contributor.

It concerns me that changes can be made (regardless of size) that impact my ability to contribute and understand the direction / priorities of the project.

I would feel more comfortable if these conversations could be made public in some manor otherwise, it could lead to users withdrawing from the project and going back to what "worked".

bogdandrutu commented 2 years ago

@MovieStoreGuy what more than the issue itself is needed? I think this is pretty public and asking for feedback, but instead @alolita derived the discussion and we are discussing process in an issue that was designed for discussing this change.

@Aneurysm9 @alolita the "internal" project was a random example, and clearly we don't need this change to do that. We can simply ignore the service dependency if we are really blocked by this. And was sure once I specify that I discussed with @tigrannajaryan you are going to finger point me about Splunk etc...

So let's discuss just about the issue from now here, if you want to discuss process please open a different issue:

MovieStoreGuy commented 2 years ago

Sorry, I don't see an issue with the change itself but more in the manor that this thread reads.

bogdandrutu commented 2 years ago

@MovieStoreGuy that is why I think we should not merge process discussions with issues/technical discussions, since it is very hard to follow.

Aneurysm9 commented 2 years ago

See https://github.com/open-telemetry/opentelemetry-collector/pull/4625 for a proposed policy statement.

As for this issue and its associated PRs, the first associated PR was merged within five minutes of the issue being opened with code committed only a minute after it was opened. I don't think it is fair to say that this issue was asking for feedback or designed to discuss these changes. It appears that this issue was created after work had begun and has only become a place for discussion because questions were asked before the most recent PR that mentions it was merged.

As for what this change is intended to do, and what its description states, there may be no disparity in the mind of its author but I don't think that it is/was clear to others of us what the goal of the change was, why it was necessary, or what alternatives had been considered and why this approach was selected over them. I think that, to the extent we are moving around types that are critical to the successful operation of a collector instance, it is fair to expect that such a proposed change would come with documentation making clear the answers to all of those questions.

Bogdan's comment goes a long way toward answering those questions in a way that is understandable to those of us who do not have the same context loaded in their brains, but it also raises additional questions. There is repeated mention of a '"config" go module' and a '"service" go module'. These do not exist at present, is it anticipated that they will be created? Doing so would be counter to the versioning policy adopted by this repository, which states that:

A single module should exist, rooted at the top level of this repository, that contains all packages provided for use outside this repository.

If we're going to change that, then we should have that discussion after a proposal is presented that outlines the rationale and expected consequences, not as an afterthought. If this issue is created with the expectation that such modules will exist and is not necessary if they do not (which is not clear to me either way at this point), then it should probably be deferred until a consensus has been reached on a change to the versioning policy.

bhs commented 2 years ago

I hear @bogdandrutu's request to keep the discussion of the actual change separated from the process-focused discussion. Where's the right place to discuss the process? #4625?

bogdandrutu commented 2 years ago

@bhs nice to see you, in our repo. the issues are not created, will do it.

bogdandrutu commented 2 years ago

For @Aneurysm9 comments related to the issue and not the process:

There is repeated mention of a '"config" go module' and a '"service" go module'. These do not exist at present, is it anticipated that they will be created? Doing so would be counter to the versioning policy adopted by this repository, which states that:

This was my bad, I was very frustrated with other's comments and I made a confusion. I was trying to say "package". I am not suggesting any split, but I want clear boundaries between "packages", and in the future if we do extract modules out of these packages that may apply as well, but I am not suggesting that.

tigrannajaryan commented 2 years ago

(I have commented process-wise on https://github.com/open-telemetry/opentelemetry-collector/pull/4625, so won't discuss that here anymore).

Here are the 2 use cases I have in my mind that would benefit from this change:

Essentially, both of the use cases can be summarized as "ability to use existing components in new interesting ways without taking the entire Collector as a dependency". Note that this does not place any additional burden on us as maintainers since component API is already part of our contract.

Also, regardless of these use cases, I believe it is generally beneficial to reduce the coupling between the Service and the code which defines what are components, what the component configuration looks like. I think this change is very much inline with a number of recent changes that were done in the last few months to cleanup our codebase and reduce the coupling between different parts. It generally improves the quality, readability and maintainability of the codebase.

Aneurysm9 commented 2 years ago

Essentially, both of the use cases can be summarized as "ability to use existing components in new interesting ways without taking the entire Collector as a dependency". Note that this does not place any additional burden on us as maintainers since component API is already part of our contract.

I think this brings us back to the question I asked earlier about whether the config and service package hierarchies were intended to be split into separate modules. To the extent that there exists a single module that contains both hierarchies I'm not sure there is a way to use one without taking a dependency on the entire collector module. This is why we have very many modules in the Go API/SDK.

If we want to change this repository's policy and begin shipping discrete modules, I'm all for having that discussion and would likely be in favor of doing so. But that is not something we can just do without discussion among stakeholders and building consensus that it is the right thing to do.

Also, regardless of these use cases, I believe it is generally beneficial to reduce the coupling between the Service and the code which defines what are components, what the component configuration looks like. I think this change is very much inline with a number of recent changes that were done in the last few months to cleanup our codebase and reduce the coupling between different parts. It generally improves the quality, readability and maintainability of the codebase.

Reduced coupling is indeed a good goal, but I'm not sure this achieves that. This set of changes has actually introduced coupling flowing from config to service where none existed before and the remaining PR will add yet more. Will that coupling remain, or is #4608 not, in fact, the last PR needed to achieve this goal. If not, what remains and what is the plan?

I'm not saying that this is a bad change. I'm saying that it is hard to evaluate the change in the absence of information regarding the intent and future plans. If there are no future plans then I cannot approve #4608 because it doesn't do what this issue seems to say it should do. If there are future plans, I want to have them documented so that the contributors to this project can understand where it is headed and we can have discussion of them with full context and on a level playing field. Information asymmetry does not help anyone here.

bogdandrutu commented 2 years ago

Will that coupling remain, or is #4608 not, in fact, the last PR needed to achieve this goal. If not, what remains and what is the plan?

To answer very directly:

  1. configtest the deps are deprecated - so expect them to be removed in the next release (following kind of a process here). See https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtest/configtest.go#L36
  2. configunmarshaler needs to be split as well into helpers to unmarshal components vs the service and whole Config.

I'm saying that it is hard to evaluate the change in the absence of information regarding the intent and future plans

In the issue we should evaluate the main goal, if we want to get into implementation details fine. But it seems that the overall goal is understood correct?

Aneurysm9 commented 2 years ago

We're beyond concepts, there are concrete implementations committed to the mainline, so yes, I think we need to get into implementation details. What is the plan for configunmarshaler?

bogdandrutu commented 2 years ago

@Aneurysm9 I will add you to the PR to review and develop a plan. What I know right now is that needs to be split as well.