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

Add system uptime metric #1507

Merged
merged 1 commit into from
Oct 25, 2024
Merged

Add system uptime metric #1507

merged 1 commit into from
Oct 25, 2024

Conversation

kevinnoel-be
Copy link
Contributor

Fixes #648

Changes

Add system.uptime metric

Merge requirement checklist

@kevinnoel-be kevinnoel-be requested review from a team as code owners October 22, 2024 11:51
@kevinnoel-be kevinnoel-be mentioned this pull request Oct 22, 2024
Closed
@mx-psi mx-psi requested a review from braydonk October 22, 2024 11:57
@trask
Copy link
Member

trask commented Oct 22, 2024

hi @kevinnoel-be! it may be worth looking over open-telemetry/oteps#185

@kevinnoel-be kevinnoel-be requested a review from lmolkova October 23, 2024 10:00
@ChrsMark ChrsMark mentioned this pull request Oct 23, 2024
Closed
braydonk
braydonk reviewed Oct 23, 2024
View reviewed changes
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

LGTM, this was pretty much what I was going to do. I do have an open question to semconv maintainers, depending on their answer may ask for a change or approve.

model/system/metrics.yaml Outdated Show resolved Hide resolved
model/system/metrics.yaml Outdated Show resolved Hide resolved
braydonk
braydonk reviewed Oct 23, 2024
View reviewed changes
model/system/metrics.yaml Outdated Show resolved Hide resolved
@kevinnoel-be kevinnoel-be force-pushed the add-system-uptime branch 4 times, most recently from ded7632 to 474b5ab Compare October 23, 2024 16:38
lmolkova
lmolkova approved these changes Oct 23, 2024
View reviewed changes
model/system/metrics.yaml Outdated Show resolved Hide resolved
lmolkova
lmolkova reviewed Oct 23, 2024
View reviewed changes
model/system/metrics.yaml Outdated Show resolved Hide resolved
@jsuereth
Copy link
Contributor

Just want to call out that we have some previous thoughts and rationale here: open-telemetry/oteps#185

braydonk
braydonk reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Based on open-telemetry/oteps#185 and the response from the semconv maintainers, I'm requesting the following changes.

model/system/metrics.yaml Outdated Show resolved Hide resolved
model/system/metrics.yaml Show resolved Hide resolved
jsuereth
jsuereth reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

As braydonk@ stated, I think the system.uptime should be a gauge, which DOES deviate from other time-related values for specific reasons, which is why we wrote the OTEP oh so long ago.

Otherwise looks good.

docs/system/system-metrics.md Outdated Show resolved Hide resolved
@kevinnoel-be
Copy link
Contributor Author

Adressed all the comments, PTAL again, thanks!

@trask trask mentioned this pull request Oct 25, 2024
Closed
@lmolkova lmolkova merged commit 6da7369 into open-telemetry:main Oct 25, 2024
13 of 14 checks passed
@kevinnoel-be kevinnoel-be deleted the add-system-uptime branch October 25, 2024 16:51
kevinnoel-be added a commit to collibra/opentelemetry-collector-contrib that referenced this pull request Oct 28, 2024
@kevinnoel-be
switch system.uptime to gauge
5d9b9bb 
according to semantic convention, see discussion in: open-telemetry/semantic-conventions#1507
@kevinnoel-be kevinnoel-be mentioned this pull request Oct 28, 2024
Merged
@ChrsMark ChrsMark mentioned this pull request Nov 26, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmolkova lmolkova lmolkova approved these changes

@trask trask trask approved these changes

@jsuereth jsuereth jsuereth approved these changes

@braydonk braydonk Awaiting requested review from braydonk

Assignees
No one assigned
Labels
None yet
Projects
Semantic Conventions Triage
Archived in project
System Semantic Convention Working Group
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add system uptime metric
5 participants
@kevinnoel-be @trask @jsuereth @lmolkova @braydonk