feat: add status label by naseemkullah · Pull Request #624 · open-telemetry/opentelemetry-js

naseemkullah · 2019-12-15T17:52:46Z

Which problem is this PR solving?

To help users getting started to have a status label for requests when using metrics. This is a very useful label to have when counting requests as to calculate error ratios.

ref.: https://landing.google.com/sre/workbook/chapters/alerting-on-slos/

Short description of the changes

Added a status label that comes from res.statusCode to all counted requests

codecov-io · 2019-12-15T17:57:55Z

Codecov Report

Merging #624 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
- Coverage   92.31%   92.22%   -0.09%     
==========================================
  Files         177      183       +6     
  Lines        8728     9118     +390     
  Branches      758      798      +40     
==========================================
+ Hits         8057     8409     +352     
- Misses        671      709      +38

Impacted Files	Coverage Δ
...kages/opentelemetry-plugin-dns/test/utils/utils.ts	`33.33% <0%> (-26.67%)`	⬇️
...opentelemetry-base/test/resources/resource.test.ts	`100% <0%> (ø)`	⬆️
...telemetry-plugin-grpc/test/utils/assertionUtils.ts	`100% <0%> (ø)`	⬆️
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts	`100% <0%> (ø)`	⬆️
...try-plugin-dns/test/functionals/dns-enable.test.ts	`100% <0%> (ø)`	⬆️
...telemetry-scope-base/test/NoopScopeManager.test.ts	`100% <0%> (ø)`	⬆️
...res/opentelemetry-plugin-pg/test/assertionUtils.ts	`100% <0%> (ø)`
...postgres/opentelemetry-plugin-pg/test/testUtils.ts	`15% <0%> (ø)`
...ugin-postgres/opentelemetry-plugin-pg/src/utils.ts	`96.96% <0%> (ø)`
...-plugin-postgres/opentelemetry-plugin-pg/src/pg.ts	`91.37% <0%> (ø)`
... and 3 more

naseemkullah · 2019-12-15T18:02:05Z

Actually something is wrong, requests that should be 404 are counted as 200

Please do not merge.

% curl localhost:3000/badurl   
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Cannot GET /badurl</pre>
</body>
</html>

returns a 404 however:

% curl localhost:9464/metrics
# HELP requests Count all incoming requests
# TYPE requests counter
requests{route="/badurl",status="200"} 1

counts it as a 200

dyladan

If you are going to modify the getting started code, make sure you add a line in the guide itself explaining why.

naseemkullah · 2019-12-16T14:30:21Z

If you are going to modify the getting started code, make sure you add a line in the guide itself explaining why.

Will do! Any idea as to why every status label gets a 200 value despite of actual response status code?

obecny · 2019-12-16T14:35:26Z

Could you please add some description explaining why is this needed ?

naseemkullah · 2019-12-16T14:37:14Z

Could you please add some description explaining why is this needed ?

Yes, my mistake, initial post updated now.

dyladan · 2019-12-16T14:50:42Z

If you are going to modify the getting started code, make sure you add a line in the guide itself explaining why.

Will do! Any idea as to why every status label gets a 200 value despite of actual response status code?

Yes. The issue is that this middleware is too early in the process and the response code has not been set yet. The default is 200.

naseemkullah · 2019-12-16T14:52:02Z

If you are going to modify the getting started code, make sure you add a line in the guide itself explaining why.

Will do! Any idea as to why every status label gets a 200 value despite of actual response status code?

Yes. The issue is that this middleware is too early in the process and the response code has not been set yet. The default is 200.

Hmm I was suspecting something of the sort, thanks for confirming. Is there an easy approach to get the actual response code?

dyladan · 2019-12-16T15:02:20Z

If you are going to modify the getting started code, make sure you add a line in the guide itself explaining why.

Will do! Any idea as to why every status label gets a 200 value despite of actual response status code?

Yes. The issue is that this middleware is too early in the process and the response code has not been set yet. The default is 200.

Hmm I was suspecting something of the sort, thanks for confirming. Is there an easy approach to get the actual response code?

Not really an "easy" approach. morgan logging library achieves this by monkey patching the res.writeHead method which is called when the response starts to send. Not really "getting started" level though.

naseemkullah · 2019-12-16T15:04:06Z

Ah shucks, ok then. Thanks!

dyladan · 2019-12-16T15:07:23Z

Maybe you could use the on-response npm package which papers over the complexity of monkey patching writeHead.

naseemkullah · 2019-12-16T15:08:46Z

I'll look into that as being able to count the # of errored requests will probably be something most users want.

Add getIdGeneratorFromConfiguration helper in sdk-node that maps the config model's tracer_provider.id_generator to an SDK IdGenerator instance. Currently only `random` is supported per the spec; unknown types warn and fall back to the SDK default (RandomIdGenerator). Export IdGenerator as IdGeneratorConfigModel from the configuration package so downstream consumers can reference the type. Based on mike/bump-config-schema-prometheus-rename which bumps the schema to a SHA on main that includes IdGenerator (open-telemetry#624 upstream). Closes open-telemetry#6616 Assisted-by: Claude Opus 4.6

feat: add status label

9f441ed

naseemkullah requested review from OlivierAlbertini, bg451, dyladan, markwolff, mayurkale22, obecny, rochdev and vmarchaud as code owners December 15, 2019 17:52

vmarchaud approved these changes Dec 15, 2019

View reviewed changes

dyladan reviewed Dec 16, 2019

View reviewed changes

naseemkullah closed this Dec 16, 2019

naseemkullah deleted the expand-metrics-example branch December 16, 2019 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add status label#624

feat: add status label#624
naseemkullah wants to merge 1 commit into
open-telemetry:masterfrom
naseemkullah:expand-metrics-example

naseemkullah commented Dec 15, 2019 •

edited

Loading

Uh oh!

codecov-io commented Dec 15, 2019 •

edited

Loading

Uh oh!

naseemkullah commented Dec 15, 2019 •

edited

Loading

Uh oh!

dyladan left a comment

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

obecny commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

dyladan commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

dyladan commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

dyladan commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

naseemkullah commented Dec 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Short description of the changes

Uh oh!

codecov-io commented Dec 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

naseemkullah commented Dec 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dyladan left a comment

Choose a reason for hiding this comment

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

obecny commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

dyladan commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

dyladan commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

dyladan commented Dec 16, 2019

Uh oh!

naseemkullah commented Dec 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

naseemkullah commented Dec 15, 2019 •

edited

Loading

codecov-io commented Dec 15, 2019 •

edited

Loading

naseemkullah commented Dec 15, 2019 •

edited

Loading