-
Notifications
You must be signed in to change notification settings - Fork 769
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
AdsCert signer metrics #2351
AdsCert signer metrics #2351
Conversation
signatureMessage, err := adsCertSigner.Sign(reqData[i].Uri, reqData[i].Body) | ||
bidder.me.RecordAdsCertSignTime(time.Since(startSignRequestTime)) |
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.
Do we expect that the time taken to generate a signature to be significant to warrant tracking as a 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.
When we enable this feature it can help to show if this feature can be a potential bottleneck in request handling. It can be especially valuable when PBS uses remote signatory service.
@SyntaxNode please confirm
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.
Only question in my mind at this point is should it always be active, or should we have a config option to turn at least the timing off? The success/failure metric should only take 2 datapoints per time step, but the timing will eat up ~20 datapoints per time step.
New metrics are added to track AdsCert signing:
RecordAdsCertReq
- record requests with AdsCert header, tracks number of successful and failed requests.RecordAdsCertSignTime
- time to sign AdsCert requestLocal test with Prometheus passed, all metrics are correctly displayed in Prometheus data