engine: report go version on startup #159
Conversation
engine.go
Outdated
| 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) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't see a way to report a string instead of a number, but I will look again
There was a problem hiding this comment.
Yeah fair, I don't know the stats stack intimately so maybe it's just not a thing. Thanks for checking.
There was a problem hiding this comment.
What if we put the version string in a tag, and set a gauge/counter to 1? Then you can sum by version tag.
kevinburkesegment
force-pushed
the
report-go-version
branch
from
9611584 to
6fea46b
Compare
kevinburkesegment
force-pushed
the
report-go-version
branch
4 times, most recently
from
99245fc to
4d3877b
Compare
kevinburkesegment
force-pushed
the
report-go-version
branch
3 times, most recently
from
9b59a64 to
ce024fe
Compare
There was a problem hiding this comment.
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
force-pushed
the
report-go-version
branch
3 times, most recently
from
4b5de05 to
267cde5
Compare
kevinburkesegment
force-pushed
the
report-go-version
branch
from
267cde5 to
808536b
Compare
engine.go
Outdated
| @@ -144,7 +149,24 @@ func (eng *Engine) ClockAt(name string, start time.Time, tags ...Tag) *Clock { | |||
| } | |||
| } | |||
|
|
|||
| var GoVersionReportingEnabled = os.Getenv("STATS_DISABLE_GO_VERSION_REPORTING") != "true" | |||
There was a problem hiding this comment.
Suggestion: setting boolean environment variables to "yes", "1", etc. is pretty common. Checking for emptiness/"false"/"f"/"0" may be safer than only looking for "true".
engine.go
Outdated
| vsn := strings.TrimPrefix(runtime.Version(), "go") | ||
| parts := strings.Split(vsn, ".") | ||
| // this filters out weird compiled Go versions like tip. | ||
| // older Go version might be "go1.13" | ||
| if len(parts) == 2 || len(parts) == 3 { | ||
| eng.measureOne(t, "go_version", 1, Gauge, []Tag{{"go_version", vsn}}...) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Doesn't really matter how many parts are in the version string now, right?
| vsn := strings.TrimPrefix(runtime.Version(), "go") | |
| parts := strings.Split(vsn, ".") | |
| // this filters out weird compiled Go versions like tip. | |
| // older Go version might be "go1.13" | |
| if len(parts) == 2 || len(parts) == 3 { | |
| eng.measureOne(t, "go_version", 1, Gauge, []Tag{{"go_version", vsn}}...) | |
| } | |
| }) | |
| } | |
| vsn := strings.TrimPrefix(runtime.Version(), "go") | |
| eng.measureOne(t, "go_version", 1, Gauge, []Tag{{"go_version", vsn}}...) | |
| }) | |
| } |
netstats/listener_test.go
Outdated
| vsn := strings.TrimPrefix(runtime.Version(), "go") | ||
| parts := strings.Split(vsn, ".") | ||
| measures := h.Measures() | ||
| if len(parts) == 2 || len(parts) == 3 { | ||
| if len(measures) != 1+1 { | ||
| t.Fatalf("expecting to get %d metrics, got back %d: %v", 1+1, len(measures), measures) | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this tests the new implementation?
kevinburkesegment
force-pushed
the
report-go-version
branch
from
808536b to
53208e5
Compare
kevinburkesegment
force-pushed
the
report-go-version
branch
5 times, most recently
from
524ce55 to
ad56b2a
Compare
kevinburkesegment
force-pushed
the
report-go-version
branch
2 times, most recently
from
50672eb to
1924b42
Compare
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
force-pushed
the
report-go-version
branch
2 times, most recently
from
e9608fd to
f81e32d
Compare
Turn off the thing where the tests can pass even though lint fails - it's as good as not running the linter at all. The "stats" name import next to "github.com/segmentio/stats/v5" is not necessary but was prompted by golangci-lint at one point.
kevinburkesegment
force-pushed
the
report-go-version
branch
from
f81e32d to
4b63042
Compare
The first time you report any metric, additionally report the running Go version. Add two flags that can be used to disable this behavior.