-
Notifications
You must be signed in to change notification settings - Fork 149
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 otel components command #6539
Add otel components command #6539
Conversation
This pull request does not have a backport label. Could you fix it @ChrsMark? 🙏
|
|
5c51c6d
to
a8d4dfb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a simple integration test to verify that this actually works?
Also, could we use upstream's implementantion here instead of copying it?
I didn't find an obvious way to use it directly from I see our
Shall we test against the real EDOT's output? This would mean that every time we include a new component we will need to update that test too. |
Allright, let's leave it as is then.
I was just thinking about checking if the output looks reasonable, without enumerating the components, but maybe that's a good idea? I'm not sure if we have an integration test that verifies which components are present right now. |
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
Another thing I see that is missing here is the
Is this something that we should fix or we can just skip it from the output for now? Any thoughts? |
I'd skip it for now and open an issue to include it. We should be as close to builder output as possible. |
4f35b9c
to
b617415
Compare
All right, I have removed the Feel free to take a look when you get the time. |
I wonder if the coverage failure is legit: #6539 (comment). It complains that the new |
@ChrsMark we had similar problem in the past hence the reason why it became a non required check. Feel free to ignore it. |
Can we mention the command in the OTel README? I think it would help users make sure the command is supported. A single sentence with an example, similar to the one with
|
3d1f3b3
to
63ba927
Compare
Done! |
@andrzej-stencel @michalpristas @swiatekm I have enabled auto-merge but I guess that's not going to work without |
@ChrsMark it will merge without SonarQube, you have a different failure coming from |
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
ec8a220
to
4cbb711
Compare
Quality Gate failedFailed conditions |
* Add otel components command Signed-off-by: ChrsMark <chrismarkou92@gmail.com> * fixes and test addition Signed-off-by: ChrsMark <chrismarkou92@gmail.com> * add note about components command in README Signed-off-by: ChrsMark <chrismarkou92@gmail.com> * fix fmt Signed-off-by: ChrsMark <chrismarkou92@gmail.com> --------- Signed-off-by: ChrsMark <chrismarkou92@gmail.com> Co-authored-by: Andrzej Stencel <andrzej.stencel@elastic.co> (cherry picked from commit be4ee59)
What does this PR do?
This PR adds the
components
command forotel
mode.Implementation is replicated from https://github.com/open-telemetry/opentelemetry-collector/blob/main/otelcol/command_components.go.
Why is it important?
otel components
command can be used to list the supported components the EDOT includes.Checklist
./changelog/fragments
using the changelog toolDisruptive User Impact
How to test this PR locally
Build Elastic Agent and execute the command:
Related issues
Questions to ask yourself