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.

Sign up for GitHub

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

Jump to bottom

allow empty metric units in mdatagen #27089

Closed
povilasv opened this issue Sep 25, 2023 · 7 comments
Closed

allow empty metric units in mdatagen #27089

povilasv opened this issue Sep 25, 2023 · 7 comments
Labels
cmd/mdatagen mdatagen command enhancement New feature or request

Comments

@povilasv
Copy link
Contributor

Component(s)

cmd/mdatagen

Is your feature request related to a problem? Please describe.

Some metrics don't need units. See #10553 (comment)

At the moment we don't allow empty metric units:

failed loading /home/povilasv/gocode/src/github.com/open-telemetry/opentelemetry-collector-contrib/receiver/k8sclusterreceiver/metadata.yaml:
metric "k8s.container.ready": missing metric unit; metric "k8s.pod.status_reason": missing metric unit; metric "k8s.namespace.phase": missing metric unit; metric "k8s.pod.phase": missing metric unit
doc.go:4: running "mdatagen": exit status 1

Describe the solution you'd like

Passing flag, or disabling validation. Both seem vialble options.

See discussion here: #10553 (comment)

Describe alternatives you've considered

No response

Additional context

No response
@povilasv povilasv added enhancement New feature or request needs triage New item requiring triage labels Sep 25, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the cmd/mdatagen mdatagen command label Sep 25, 2023
@povilasv povilasv mentioned this issue Sep 25, 2023
Merged
@mx-psi
Copy link
Member

mx-psi commented Sep 25, 2023

AIUI UCUM defines '1' as the default unit. Why can't we just use 1?

@povilasv
Copy link
Contributor Author

According to this comment #10553 (comment)

The Enum type metric should have empty units

IMPORTANT: Unit must be empty, and metric type must be UpDownCounter (users will typically use sum(k8s.container.ready) to count the number of containers that are ready).

cc @bertysentry

The discussion below it also has more context.

@mx-psi
Copy link
Member

mx-psi commented Sep 25, 2023

Let's continue the discussion on #10553 first, I am not convinced that we can't (or shouldn't) use 1.

@povilasv
Copy link
Contributor Author

Agree, converted PR to draft. Thanks for looking into it

@crobert-1 crobert-1 added the on hold This is blocked by another PR/issue label Sep 25, 2023
@crobert-1 crobert-1 mentioned this issue Sep 25, 2023
Merged
@github-actions github-actions bot mentioned this issue Sep 26, 2023
Open
@crobert-1
Copy link
Member

From conversation on #10553, it looks like we'll go ahead with the no unit approach in this scenario. Removing the on hold label.

@crobert-1 crobert-1 removed the on hold This is blocked by another PR/issue label Sep 27, 2023
@povilasv
Copy link
Contributor Author

PR is open if anyone wants to help review #27090 :)

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Sep 27, 2023
dmitryax pushed a commit that referenced this issue Sep 29, 2023
@povilasv
[mdatagen] allow to skip metric unit validation (#27090)
2c94210 
**Description:** <Describe what has changed.>

Allows setting empty metric units in mdatagen.

Example of using it
https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/27091/files


**Tracking Issues:**

#27089

#10553
@povilasv povilasv closed this as completed Oct 4, 2023
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
@povilasv @jmsnll
[mdatagen] allow to skip metric unit validation (open-telemetry#27090)
f39e5ab 
**Description:** <Describe what has changed.>

Allows setting empty metric units in mdatagen.

Example of using it
https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/27091/files


**Tracking Issues:**

open-telemetry#27089

open-telemetry#10553
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cmd/mdatagen mdatagen command enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mx-psi @povilasv @crobert-1