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 a collector flag to list available components. #5710

Closed
wants to merge 1 commit into from

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Jul 20, 2022

Description:

Add a --list-components flag to the collector service. This currently
show all the available components, their types and their signal stability
levels. In future, we could also show the Go module and version, but
that would require changes to the builder, so it's omitted for now.

Link to tracking Issue:

This fixes #5709.

Testing:

Just manual.

otelcorecol jpeach$ go build . && ./otelcorecol --list-components
exporters:
	logging (traces: in development, metrics: in development, logs: in development)
	otlp (traces: stable, metrics: stable, logs: beta)
	otlphttp (traces: stable, metrics: stable, logs: beta)
receivers:
	otlp (traces: stable, metrics: stable, logs: beta)
processors:
	batch (traces: stable, metrics: stable, logs: stable)
	memory_limiter (traces: beta, metrics: beta, logs: beta)
extensions:
	memory_ballast
	zpages

Documentation:

None. Visible in usage output.

@jpeach jpeach requested review from a team and bogdandrutu July 20, 2022 06:06
service/command.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -10,6 +10,7 @@
- Expose `pcommon.NewSliceFromRaw` function (#5679)
- `loggingexporter`: create the exporter's logger from the service's logger (#5677)
- Add `otelcol_exporter_queue_capacity` metrics show the collector's exporter queue capacity (#5475)
- Add the Collector flag `--list-components` to show the available components. (#5709)
Copy link
Member

Choose a reason for hiding this comment

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

This does not look like a flag to me, but like a subcommand (something like build-info, components or show-components?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh I thought about that, but it was more intrusive, and I didn't want to introduce a new thing that the collector wasn't already doing.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's likely we will eventually have subcommands, some other FRs also make sense as subcommands, like #5654 or #5223. But I agree that it's a significant addition, we may want to put this behind a feature gate to signal that we haven't yet fully figured out how the final design will look like.

@mx-psi
Copy link
Member

mx-psi commented Jul 20, 2022

Also this overlaps somewhat with what the zpages extension does. It may be a better idea to add it there, rather than on a subcommand, which is something we haven't done before.

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Base: 91.59% // Head: 91.62% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (d699f93) compared to base (fa45027).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5710      +/-   ##
==========================================
+ Coverage   91.59%   91.62%   +0.03%     
==========================================
  Files         217      217              
  Lines       13425    13477      +52     
==========================================
+ Hits        12296    12348      +52     
  Misses        906      906              
  Partials      223      223              
Impacted Files Coverage Δ
service/command.go 81.25% <100.00%> (+26.70%) ⬆️
service/flags.go 83.87% <100.00%> (+3.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jpeach
Copy link
Contributor Author

jpeach commented Jul 20, 2022

Also this overlaps somewhat with what the zpages extension does. It may be a better idea to add it there, rather than on a subcommand, which is something we haven't done before.

Yeh, this probably makes sense in zpages. However, what I was looking for here is a quick way to see what I have in a collector binary. zpages makes more sense when I have a fleet of collectors.

@jpeach jpeach force-pushed the collector-list-components branch from f4d6eb3 to b96961c Compare July 20, 2022 06:22
@bogdandrutu
Copy link
Member

Since we all agree that zpages is a must, let's do that and if we find out that is annoying for this case we can revisit. I feel that we are overthinking, let's start with what we know for sure is needed.

@jpeach
Copy link
Contributor Author

jpeach commented Jul 20, 2022

Since we all agree that zpages is a must, let's do that

I think that's a separate PR. It doesn't work for my use case, which is to check which components are in a binary. zpages might not be enabled in the build. To use the data I have to know that zpages is a thing and have read it's documentation, then configure and run the collector as a service and curl it, which is a lot more effort that I think is necessary, and not a good UX for people who are new to the ecosystem.

@jpeach
Copy link
Contributor Author

jpeach commented Jul 30, 2022

Since we all agree that zpages is a must, let's do that

I think that's a separate PR. It doesn't work for my use case, which is to check which components are in a binary. zpages might not be enabled in the build. To use the data I have to know that zpages is a thing and have read it's documentation, then configure and run the collector as a service and curl it, which is a lot more effort that I think is necessary, and not a good UX for people who are new to the ecosystem.

@bogdandrutu so if I'm new to the otel ecosystem, I probably pull down a prebuilt collector binary. I start reading about how I need to configure receivers and exporters and all these optional components. The first thing I need to know is which components I have. IMHO zpages doesn't help this at all.

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Aug 13, 2022
@jpeach
Copy link
Contributor Author

jpeach commented Aug 16, 2022

not stale

@jpeach jpeach force-pushed the collector-list-components branch from b96961c to 9353eb9 Compare August 16, 2022 02:55
@github-actions github-actions bot removed the Stale label Aug 16, 2022
@TylerHelmuth
Copy link
Member

I agree with @jpeach that a subcommand/flag is more useable for this use case. Zpages requires the collector to be running with a valid configuration (that includes the extension) and then you have to know how to use zpages to get to the endpoint that lists all the components. For a brand new user this is not a user-friendly.

I believe that it is a better user experience to directly ask the collector "what components can I use" without first needing to pass it a valid config (which requires knowledge of what components can be used). Relying on Zpages also requires the binary to include the Zpages extension, which custom distributions may not include.

If we really can't add a subcommand or another flag could we at least add the component list to the --help flag?

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

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

@github-actions github-actions bot added the Stale label Sep 7, 2022
@mx-psi mx-psi removed the Stale label Sep 7, 2022
@jpeach jpeach force-pushed the collector-list-components branch from 9353eb9 to bdc1070 Compare September 11, 2022 23:42
@jpeach
Copy link
Contributor Author

jpeach commented Sep 11, 2022

Rebased again. @open-telemetry/collector-approvers can we get consensus here?

@jpkrohling
Copy link
Member

I believe that it is a better user experience to directly ask the collector "what components can I use" without first needing to pass it a valid config

This has my vote.

@jpeach jpeach force-pushed the collector-list-components branch from bdc1070 to dbc3e57 Compare September 13, 2022 03:59
Add a `--list-components` flag to the collector service. This currently
show all the available components, their types and their signal stability
levels. In future, we could also show the Go module and version, but
that would require changes to the builder, so it's omitted for now.

This fixes open-telemetry#5709.

Signed-off-by: James Peach <jpeach@cloudflare.com>
@jpeach jpeach force-pushed the collector-list-components branch from dbc3e57 to d699f93 Compare September 15, 2022 01:00
@TylerHelmuth
Copy link
Member

@jpeach if we don't make progress on this in the next week I'll add this PR to the SIG meeting next Wednesday.

@bogdandrutu
Copy link
Member

@TylerHelmuth @jpeach @jpkrohling please provide exactly the commands/flags that you want to add:

  • Do you list per component type?
  • Do we need to support this for windows service handler? What is the separation of things supported for windows service handler vs non windows service? Needs to better understand this.
  • Do you have a flag or subcommand? I prefer subcommand. If subcommand probably we should also have a "run" subcommand for current "run" behavior of the root command and deprecate the current root. Also if the decision for flag is just because we are worried about the followup work, I am not ok with that.

@jpeach
Copy link
Contributor Author

jpeach commented Sep 20, 2022

please provide exactly the commands/flags that you want to add

Umm, that's in the patch and the description field for this PR?

Do you list per component type?

Do you mean a flag that would let you list one type of component and not others? No, the behavior I'm proposing is what's in the PR.

Do we need to support this for windows service handler?

What is this? If windows has collector components, then this would list them.

Do you have a flag or subcommand?

It's a flag, because the implementation is trivial and doesn't require a lot of refactoring or backwards compatibility.

@bogdandrutu
Copy link
Member

What is this? If windows has collector components, then this would list them.

https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/collector_windows.go#L45

It's a flag, because the implementation is trivial and doesn't require a lot of refactoring or backwards compatibility.

I think I already answered to this.

Also if the decision for flag is just because we are worried about the followup work, I am not ok with that.

@jpeach
Copy link
Contributor Author

jpeach commented Sep 21, 2022

OK, sounds like we don't have consensus for this change.

@jpeach jpeach closed this Sep 21, 2022
@jpkrohling
Copy link
Member

Do you have a flag or subcommand? I prefer subcommand

Not a strong preference, but as a user, I would prefer a flag like '--help' (as I see this as a specialization of '--help'). When looking at the binary, I know it's for running a collector, but before doing that, I might just want to check what's the version and which components it has.

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.

Show which components are built into a collector
5 participants