-
Notifications
You must be signed in to change notification settings - Fork 751
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
New Metrics: Open Telemetry #4065
base: master
Are you sure you want to change the base?
Conversation
@@ -1,10 +1,13 @@ | |||
module github.com/prebid/prebid-server/v3 | |||
|
|||
go 1.21 | |||
go 1.22 |
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.
This should be extracted to a separate PR. We release Go upgrades separately to isolate potential issues introduced by the runtime.
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 see - ok then we will need to make a new one for that as prerequisite for this; will mark as draft until that time.
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.
metrics/config/metrics.go
Outdated
@@ -1,10 +1,12 @@ | |||
package config | |||
|
|||
import ( | |||
"log/slog" |
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.
Please continue to use glog for this PR. We'll discuss changing logging libraries in a separate thread.
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 - sounds good; filed #4103 for that
} | ||
|
||
// InitMetrics initializes all metrics in the given struct (except those tagged with metric="-"). | ||
func InitMetrics(meter metric.Meter, m any, prefix string) error { |
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.
Curious, is this a required step or a pre-optimization similar to what we have for Prometheus?
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.
Similar to what you have for prometheus, in this case, it takes the nuisance about calling the initializer and checking errors for every bleeping metric by using reflection to do the necessary and choosing the name from its member name and hierarchy.
It's called by NewMetrics in engine.go
return nil, err | ||
} | ||
return ret, nil | ||
} |
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.
How are OpenTelemetry metrics published?
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.
They are published by the configuration of the sdk - this only hooks up the otel interfaces and does not provide configuration of the sdk. If a host, using prebid as a library/module, configures otel, then these metrics will go wherever it specifies.
This change adds an open telemetry
MetricsEngine