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

metric_readers section name could be shortened to readers #20

Closed
mx-psi opened this issue Jun 26, 2023 · 2 comments · Fixed by #22
Closed

metric_readers section name could be shortened to readers #20

mx-psi opened this issue Jun 26, 2023 · 2 comments · Fixed by #22
Assignees
Labels
question Further information is requested

Comments

@mx-psi
Copy link
Member

mx-psi commented Jun 26, 2023

TL;DR

The metric_readers section can be shortened to readers which would be shorter while still being readable and concrete enough, and would make the OpenTelemetry Collector telemetry configuration cleaner.

Longer explanation & context

On open-telemetry/opentelemetry-collector/pull/7871 we are using the Configuration WG's meter_provider configuration schema as part of the OpenTelemetry Collector telemetry configuration. In this case, configuring a reader looks like the following:

# collector.yaml sample telemetry configuration
service:
  telemetry:
    metrics:
      metric_readers:
        pull/prometheus:
          # ...

As noted, this matches the example on this repository:

meter_provider:
metric_readers:
# Add a pull-based metric reader.
pull/prometheus:
exporter:
prometheus:

Although I believe in our case it is specially more salient because our top level configuration is named metrics, I still think on the base example on this repository the metric_ prefix for the metric_readers subsection is redundant: a meter provider will presumably never have any other kind of reader.

@codeboten
Copy link
Contributor

The name metric_reader was chosen as a way to remain consistent w/ the definition in the specification. After reviewing some of the other fields in the current configuration, it is clear that there are currently inconsistencies already. For example exporter under the pull/prometheus/ should be named metric_exporter. Although we could go the route of prefixing everything here, this would make the configuration quite repetitive.

After a discussion in the working group call this morning, the proposal is to drop the following prefixes under each provider:

  • For meter provider: metric_
  • For tracer provider: span_
  • For logger provider: logrecord_

This would allow us to create a consistent rule to make expectations clear for users. WDYT @mx-psi?

@mx-psi
Copy link
Member Author

mx-psi commented Jun 26, 2023

@codeboten That makes sense to me 👍 In all cases in the current schema I believe everything is still readable while being shorter after dropping those prefixes

@codeboten codeboten self-assigned this Jun 26, 2023
codeboten pushed a commit to codeboten/opentelemetry-configuration that referenced this issue Jun 26, 2023
Closes open-telemetry#20

Signed-off-by: Alex Boten <aboten@lightstep.com>
codeboten pushed a commit that referenced this issue Jun 27, 2023
Closes #20
---------
Signed-off-by: Alex Boten <aboten@lightstep.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants