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

engine: report go version on startup #159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kevinburkesegment
Copy link
Contributor

The first time you report any metric, additionally report the running Go version. Add two flags that can be used to disable this behavior.

README.md Outdated Show resolved Hide resolved
engine.go Outdated
Comment on lines 155 to 163
if len(parts) == 2 || len(parts) == 3 {
maj, err := strconv.Atoi(parts[0])
if err == nil {
eng.measure_(t, "go_version.major", maj, Gauge)
}
min, err := strconv.Atoi(parts[1])
if err == nil {
eng.measure_(t, "go_version.minor", min, Gauge)
}
if len(parts) == 3 {
patch, err := strconv.Atoi(parts[2])
if err == nil {
eng.measure_(t, "go_version.patch", patch, Gauge)
}
}
}
Copy link

Choose a reason for hiding this comment

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

Would it be sufficient to just ship the version string (but keep that trimmed prefix, good looking out) and let folks on the consumer side manage/mangle/parse the value as they see fit? I'm not sure Major or Minor have much utility in isolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see a way to report a string instead of a number, but I will look again

Copy link

Choose a reason for hiding this comment

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

Yeah fair, I don't know the stats stack intimately so maybe it's just not a thing. Thanks for checking.

Choose a reason for hiding this comment

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

What if we put the version string in a tag, and set a gauge/counter to 1? Then you can sum by version tag.

ABHINAV-SUREKA
ABHINAV-SUREKA previously approved these changes Jun 26, 2024
Copy link

@mckern mckern left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. I've gone over this a bit, but I'm kind of torn on this implementation because while code that exists is generally preferable to code that doesn't, I'm still not convinced that this should be shipped as 3 separate values using gauges.

What you turned up from the procstats subpackage may be what we really need for our use-cases.

@kevinburkesegment kevinburkesegment force-pushed the report-go-version branch 2 times, most recently from 46595ae to 4b5de05 Compare September 9, 2024 21:49
The first time you report any metric, additionally report the running
Go version. Add two flags that can be used to disable this behavior.
@kevinburkesegment
Copy link
Contributor Author

@etiennep @mckern I reworked this to just publish a single metric with a value of "1" and the go version as a tag.

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.

4 participants