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

Rename system.processes.* namespace to system.process.* #484

Merged
merged 11 commits into from
Dec 20, 2023

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Nov 2, 2023

Fixes #266

Changes

This PR renames system.processes.* namespace to system.process.*. This is based on #267 to align with "Metric namespaces should not be pluralized" rule.

Also it changes system.processes.created to system.process.created_total since .process.created would not indicate the total number of processes but possible when a process was created.

Merge requirement checklist

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark self-assigned this Nov 2, 2023
@ChrsMark ChrsMark requested review from a team November 2, 2023 09:17
@ChrsMark ChrsMark requested a review from a team November 2, 2023 09:27
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM but needs a few updates to make CI pass

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
model/metrics/system-metrics.yaml Outdated Show resolved Hide resolved
model/metrics/system-metrics.yaml Show resolved Hide resolved
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
@joaopgrassi
Copy link
Member

@open-telemetry/semconv-system-approvers please take another look

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
mx-psi
mx-psi previously requested changes Dec 1, 2023
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Blocking to avoid merge since we want to discuss this with semconv after the WG meeting yesterday

@mx-psi
Copy link
Member

mx-psi commented Dec 4, 2023

One of the points of discussion was the fact that the hostmetrics receiver has different scrapers process and processes https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/hostmetricsreceiver#getting-started and that this rename causes the metrics to have a namespace that does not match its scraper. Some previous discussion about the rationale for the namespace can be found at #212 (comment)

@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 6, 2023

@trask any thoughts on #484 (comment)?

@ChrsMark
Copy link
Member Author

ChrsMark commented Dec 11, 2023

Trying to summarize our options here (outcome from discussions on last week's System WG meeting):

Option 1: Reconsider the pluralization rule for namespaces based on the inconsistency with Otel collector implantation mentioned at #484 (comment). Feedback from @trask
on this would be really valuable.

Option 2: Accept this PR. Change Otel Collector's implementation so as conventionally to be aligned with the namespace pluralization rule of SemConv. This means that we change processes and process scrapers. cc: @braydonk @dmitryax

Option 3: Accept this PR. Keep the pluralization rule as is as well as the current Collector's implementation but add a note in the rule and the collector to clarify that system.process.* is for aggregated metrics while process.* is for metrics of a single resource. This is a "lighter" version if option2.

Update:

Note that for option 3 we need to verify if it is aligned in general with how Collector uses system.* metrics. For example the Collector emits system.cpu.* metrics which are per CPU.

From my perspective the implementation detail that processes scraper will use the system.process.* namespace for aggregated process metrics of the system does not seem quite problematic. In the future the same scraper might emit additional metrics under a completely different namespace.

@open-telemetry/semconv-system-approvers could you have another look and provide your feedback here?

@mlunadia
Copy link
Contributor

mlunadia commented Dec 13, 2023

this rename causes the metrics to have a namespace that does not match its scraper

Option 1. Whilst I understand it would be nice to have consistency with the Otel collector implementation, enforcing this results in surfacing unnecessary implementation details to users.

IIUC the push for consistency of scraper need to match the namespace would apply across. There are areas where this would result in namespaces that many users would not necessarily be familiar with eg. kubelestats receiver: namespace would need to be k8s.kublet.* or k8s.kubeletstats.* (?) to match the relevant scraper when these are metrics related to node, pod, container, and volume.

Options 2&3. I don't have strong opinions on either of these two options because they are more focused on the implementation without affecting the user experience.

@jsuereth this comment is in addition to the reasoning of CPU not being plural as discussed at the last semcon SIG meeting.

@trask
Copy link
Member

trask commented Dec 13, 2023

@trask any thoughts on #484 (comment)?

the trouble seems to stem from having process.* (Process Metrics) and system.process.* (Aggregate System Process Metrics)

what do you think of naming the scrapers

  • process - capturing aggregated system.process.*, since this matches the naming for all the others, e.g. network, cpu, etc
  • process_detail - capturing process.* metrics (per-process metrics)

@ChrsMark
Copy link
Member Author

From the discussion on Thursday's System WG meeting(Dec 14th):

We are good to go with this PR. Collector(hostmetricsreceiver)'s maintainers will figure out the implementation details but from SemConv point of view there is no reason to block this change/PR.

cc: @dmitryax @braydonk @jsuereth

@braydonk
Copy link
Contributor

maintainers will figure out the implementation details

Yes, I won't get into too much detail but to clarify I am planning to merge the process and processes scraper, such that just the process scraper produces both process and system.process metrics.

@mx-psi mx-psi dismissed their stale review December 15, 2023 13:19

Discussion was resolved

@jsuereth
Copy link
Contributor

Can you rebase your CHANGELOG / Schema-next changes? They appear to have moved into the release :)

@jsuereth jsuereth merged commit 7051551 into open-telemetry:main Dec 20, 2023
9 checks passed
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
…ry#484)

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Co-authored-by: Joao Grassi <joao.grassi@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Rename system.processes.* namespace to system.process.*
10 participants