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 message about available components to AgentDetails #201

Merged
merged 16 commits into from
Oct 31, 2024

Conversation

BinaryFissionGames
Copy link
Contributor

This PR adds a new ComponentDetails type that allows agents to communicate metadata relating to the components available in the agent.

proto/opamp.proto Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated
Comment on lines 1212 to 1216
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/[email protected]": {
"metadata": {
"type": "hostmetrics",
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ID here works for how everything is structured in contrib. I'm wondering if you could cause a collision through, simply by having multiple components in one module? Do we need to support that? I don't think the builder supports that case currently, either.

Choose a reason for hiding this comment

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

Technically the builder does support this, though I don't think it's frequently used. If components are hosted under different paths inside the same module, the builder can be told to include a component per package. That is, if the module is github.com/open-telemetry/opentelemetry-collector-contrib, the path could be receiver/hostmetricsreceiver or processor/transform for the package within the module where the component is found. I think it's safe to say that these could be reported as github.com/open-telemetry/opentelemetry-collector-contrib/[...]@vX.Y.Z, though I'm not sure how this is currently handled in the Collector's reporting of each module.

If that's accounted for, the builder limits each path/package to a single component, so we're okay from that perspective.

Copy link
Contributor Author

@BinaryFissionGames BinaryFissionGames Oct 24, 2024

Choose a reason for hiding this comment

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

This is a good point.

I inverted the ID to be the type string instead; I think the type should be unique within it's class (e.g. there is only one "hostmetricsreceiver" in any given build of the agent). Then the module is in the metadata as extra information to identify the exact version of code.

specification.md Outdated Show resolved Hide resolved
@BinaryFissionGames BinaryFissionGames marked this pull request as ready for review August 26, 2024 14:00
@BinaryFissionGames BinaryFissionGames requested a review from a team August 26, 2024 14:00
@BinaryFissionGames
Copy link
Contributor Author

@andykellr @tigrannajaryan Here's a rough draft of ComponentDetails, when you get a second to take a look.

@BinaryFissionGames
Copy link
Contributor Author

@tigrannajaryan friendly bump on this

proto/opamp.proto Outdated Show resolved Hide resolved
proto/opamp.proto Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

@tigrannajaryan friendly bump on this

Sorry for the delay. Reviewed.

@tigrannajaryan
Copy link
Member

@BinaryFissionGames please ping me once this is updated to reflect our last discussion in #198

@BinaryFissionGames BinaryFissionGames requested a review from a team as a code owner October 15, 2024 17:42
specification.md Outdated Show resolved Hide resolved
specification.md Outdated
Comment on lines 1212 to 1216
"github.com/open-telemetry/opentelemetry-collector-contrib/receiver/[email protected]": {
"metadata": {
"type": "hostmetrics",
}
}

Choose a reason for hiding this comment

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

Technically the builder does support this, though I don't think it's frequently used. If components are hosted under different paths inside the same module, the builder can be told to include a component per package. That is, if the module is github.com/open-telemetry/opentelemetry-collector-contrib, the path could be receiver/hostmetricsreceiver or processor/transform for the package within the module where the component is found. I think it's safe to say that these could be reported as github.com/open-telemetry/opentelemetry-collector-contrib/[...]@vX.Y.Z, though I'm not sure how this is currently handled in the Collector's reporting of each module.

If that's accounted for, the builder limits each path/package to a single component, so we're okay from that perspective.

specification.md Show resolved Hide resolved
specification.md Show resolved Hide resolved
proto/opamp.proto Show resolved Hide resolved
@BinaryFissionGames
Copy link
Contributor Author

Looks like a link is broken unrelated to the changes in this PR, but this should be good for another look.

proto/opamp.proto Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
proto/opamp.proto Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

@andykellr or @dpaasman00 will you take over this PR to make the final fixes before merging?

@andykellr
Copy link
Contributor

@andykellr or @dpaasman00 will you take over this PR to make the final fixes before merging?

Yes, I just pushed changes based on your feedback. It should be ready.

@tigrannajaryan tigrannajaryan merged commit 75aa498 into open-telemetry:main Oct 31, 2024
5 checks passed
@andykellr andykellr deleted the feat/component-details branch October 31, 2024 14:36
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.

4 participants