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

Add native/firstParty flag to collector exporters #4836

Conversation

haidong
Copy link
Contributor

@haidong haidong commented Jul 14, 2024

To address #4795, this is a series of PRs that add native or first party flags to registries that lack them. This batch touches collector-builder and all collector-exporter-* registries.

@haidong haidong requested a review from a team July 14, 2024 22:11
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Please see my inline comment. IsFirstParty should not be used for collector components.

@@ -14,6 +14,7 @@ urls:
repo: https://github.com/open-telemetry/opentelemetry-collector/tree/main/cmd/builder
docs: /docs/collector/custom-collector/
createdAt: 2023-12-18
isFirstParty: true
Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I should have been more specific in the original issue. This flag is not relevant for collector components it refers to instrumentations that are "first party", i.e. provided by the same org that created a certain app or library.

We could consider to have them for receivers/exporters hosted outside the community repos by orgs that create such modules fur their solution but I don't know if this is what we want them to do va adopting otlp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unsure about this change. Thanks for the explanation.

I've made the adjustment to your change request, rebased and adjusted the commit message a bit, then did a force push.

Thanks again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, Do you mean that all collector-exporters don't need this change? In other words, this PR should be abandoned?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I mean, I'm ambivalent here TBH, but I don't see why it wouldn't also be flagged?

Copy link
Member

Choose a reason for hiding this comment

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

Depends on how we want to use that flag. We had a thirdParty flag in the past that we removed because it created so much confusion. Right now the first party flag is used in the registry to say what I outlined above. I don't know if we water down the value of that if all otel first party components have that label as well

To address open-telemetry#4795, this is a series of PRs that add native or first party
flags to registries that lack them. This batch touches and all
collector-exporter-* registries.

Signed-off-by: Alex Ji <[email protected]>
@haidong haidong force-pushed the native-firstParty-flag-4-collector-exporter branch from 59fc3cd to da0c3fb Compare July 15, 2024 13:13
@haidong haidong requested a review from svrnm July 15, 2024 13:18
@svrnm
Copy link
Member

svrnm commented Jul 17, 2024

Moving the discussion out from my outdated comment:

Depends on how we want to use that flag. We had a thirdParty flag in the past that we removed because it created so much confusion. Right now the first party flag is used in the registry to say what I outlined above. I don't know if we water down the value of that if all otel first party components have that label as well

I don't think that we should add the isFirstParty flag to non-instrumentation entries of the registry, especially not for things build by the opentelemetry community. Otherwise 50%+ of our entries would have this flag (it is currently not showing up because layouts/partials/ecosystem/registry/entry.html is implementing that flag exactly that way that it only works for instrumentation)

We already highlight components by the opentelemetry community by distinguishing the "by The OpenTelemetry Authors" from other others by highlighting them.

Again, @haidong, I appreciate the effort you put into creating this PR, and I apologies for the confusion that my comment in #4795 might have created, but I would like to not implement this.

@cartermp
Copy link
Contributor

Agreed, let's keep this scoped to just instrumentations then.

@haidong
Copy link
Contributor Author

haidong commented Jul 21, 2024

I've been busy with other stuff and didn't get a chance to attend to this until today.

Thanks all for your input. I've created a new PR that touch files under data/registry. Please let me know your suggestions/comments there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants