-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Metadata Generator Enhancements #1912
Metadata Generator Enhancements #1912
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1912 +/- ##
==========================================
- Coverage 91.47% 91.30% -0.17%
==========================================
Files 284 280 -4
Lines 16736 16670 -66
==========================================
- Hits 15309 15221 -88
- Misses 998 1017 +19
- Partials 429 432 +3
Continue to review full report at Codecov.
|
const SystemCPUTime = "system.cpu.time" | ||
const SystemMemoryUsage = "system.memory.usage" | ||
|
||
// MetricsByName makes it easier to look up metrics at runtime by their name. | ||
var MetricsByName = map[string]func() pdata.Metric{ | ||
SystemCPUTime: MetricFactories.SystemCPUTime, | ||
SystemMemoryUsage: MetricFactories.SystemMemoryUsage, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the constants what about changing the existing Metrics struct to work like:
Metrics.SystemCPUTime.String() or Name()
-> "system.cpu.time"
Metrics.SystemCPUTime.Create()
-> func() { return pdata.NewMetric(), etc. }
Maybe eventually it'd have:
Metrics.SystemCPUTime.Labels()
as well for the set of labels it can emit.
This keeps things closer to how Labels are structured as well.
Maybe MetricsByName
could map to the same structs, ie MetricsByName["foo.bar"].Labels()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that seems like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrcamp alright did that except I called it New
instead of Create
since that is more consistent with Go constructors in general and pdata.
@@ -73,7 +73,7 @@ func (s *scraper) ScrapeMetrics(_ context.Context) (pdata.MetricSlice, error) { | |||
} | |||
|
|||
func initializeCPUTimeMetric(metric pdata.Metric, startTime, now pdata.TimestampUnixNano, cpuTimes []cpu.TimesStat) { | |||
metadata.Metrics.SystemCPUTime.CopyTo(metric) | |||
metadata.MetricFactories.SystemCPUTime().CopyTo(metric) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this results in 2 copies being done each time (one when SystemCPUTime() is called and another in the CopyTo). Hopefully this will be replaced by metricsbuilder though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it won't have that waste with the metric builder.
const SystemCPUTime = "system.cpu.time" | ||
const SystemMemoryUsage = "system.memory.usage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to get rid of these if we could and have Metrics.SystemCPUTime.Name()
be the canonical way to get the metric name. Since we have metrics and labels all in one metadata
package it's not clear what these variables are (metrics or labels).
For below I guess you could do:
var MetricsByName = map[string]metricIntf{
Metrics.SystemCPUTime.Name(): Metrics.SystemCPUTime,
Metrics.SystemMemoryUsage.Name(): Metrics.SystemMemoryUsage,
}
(Hopefully Go would inline all those function calls but since this is code-generated could just stick the literal string in there).
or is this too verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't the module-level variables just be the interface with Name
, New
, Labels
, etc? I.e. Do we really need to nest everything in a structure called Metrics
, or can we just flatten it out to make things less verbose? I doubt there would ever be naming conflicts with the other variable names and the type system would deal with any misuse. So it seems like unless there is a reason to pass around the Metrics
struct as a whole it doesn't serve much purpose other than namespacing the metric fields.
Or alternatively maybe Metrics
could just be called M
, similar to how the testing package calls things T
and B
as a well understood convention.
I just want to keep the length of things down since in client code the reference would be metadata.Metrics.SystemMemoryUsage.Name()
which seems a bit long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would metadata.Metrics.SystemMemoryUsage.Name()
be used that much or would metadata.Metrics.SystemMemoryUsage.Create()
? (Our lint rules don't currently allow it but if you were able to import the metadata
package as import . "github.com/.../metadata"
then we could do Metrics.SystemMemoryUsage
.)
I wanted to provide some organization between metrics and labels. For example if we wanted a way to get a list of all metrics I was thinking of having metadata.Metrics.All() -> []Metric
. The other alternative was to have a package under metadata
called metrics
and labels
(though would have to make sure you don't run into package cycles) so that you would have metrics.SystemMemoryUsage
.
Could the builder library take an interface so you could pass in metadata.Metrics.SystemMemoryUsage
that adheres to an interface so you wouldn't have to call .Name() or .Create()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did another round that gets rid of the constant name strings and uses the Name()
methods, but also adds an alias for M = Metrics
and L = Labels
. That should help some with verbosity.
I would prefer shortening the package name to meta
or something before using the import .
thing so that it is more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. For the metadata thing people can always do
import m "github.com/.../metadata"
If it's too long so that part I'm not too worried about.
cmd/mdatagen/metrics.tmpl
Outdated
} | ||
|
||
// MetricFactoriesByName makes it easier to get metric factories by name. | ||
var MetricFactoriesByName = map[string]func() pdata.Metric { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is MetricFactoriesByName
needed?
MetricsByName["foo"].New()
vs MetricFactoriesByName["foo"]()
would be better I think to avoid having redundant maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ie in the future if you want to do MetricsByName["foo"].Labels()
you'd just have the 1 map instead of having LabelsByMetricName["foo"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention that for the Metric helper/builder thing, I am trying to keep the interface between this and that as minimal as possible. So the most basic and simple thing that needs is a map from metric name to a factory (func() pdata.Metric
) for it, hence the inclusion. Having a map from metric name to an interface{New() pdata.Metric}
is more complex and ties it tighter than necessary to the metadata generator.
{{- end }} | ||
} | ||
|
||
var M = Metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add comment (can just copy original comment) for M and L?
This changes the Metrics fields to a struct that implements an interface with the `New` and `Name` methods to create a new metric and get the metric name, respectively. It also adds some methods to Metrics: - `Names()` that returns a slice of all the names of metrics - `ByName(name string)` that looks up a metric by its name - `FactoriesByName()` that returns a map of factories (`New` methods) keyed by metric name. This makes it possible to refer to metrics by name in an easily comparable manner and between components without them having to explicitly know about these metadata types. It also removes the validation on the Unit field of metrics since there are many metrics that have no standard units (e.g. connections). It also adds an alias `M` that corresponds to `Metrics`, and `L` for `Labels` to shorten things where appropiriate.
@tigrannajaryan can you review as well? |
* Add passthrough example * Update example/passthrough/handler/handler.go Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update example/passthrough/README.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Robert Pająk <pellared@hotmail.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Robert Pająk <pellared@hotmail.com>
…1912) Bumps [go.uber.org/zap](https://github.com/uber-go/zap) from 1.21.0 to 1.23.0. - [Release notes](https://github.com/uber-go/zap/releases) - [Changelog](https://github.com/uber-go/zap/blob/master/CHANGELOG.md) - [Commits](uber-go/zap@v1.21.0...v1.23.0) --- updated-dependencies: - dependency-name: go.uber.org/zap dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This changes the Metrics fields to a struct that implements an interface
with the
New
andName
methods to create a new metric and get themetric name, respectively.
It also adds some methods to Metrics:
Names()
that returns a slice of all the names of metricsByName(name string)
that looks up a metric by its nameFactoriesByName()
that returns a map of factories (New
methods)keyed by metric name.
This makes it possible to refer to metrics by name in an easily
comparable manner and between components without them having to
explicitly know about these metadata types.
It also removes the validation on the Unit field of metrics since there
are many metrics that have no standard units (e.g. connections).
It also adds an alias
M
that corresponds toMetrics
, andL
forLabels
to shorten things where appropiriate.