Fix issues in TypeScript getting started example code #1374

mickdekkers · 2020-08-02T09:48:40Z

Which problem is this PR solving?

Problems in the TypeScript getting started example:

The use of a non-generic type as generic
Gaps in the TypeScript type coverage

Short description of the changes

Change the use of Metric<BoundCounter> to Counter (Metric is no longer generic, so this didn't compile)
Add @types/node so TypeScript can compile app.ts which uses process.env
Add @types/express so the express API is no longer any
Define the the result type of the countAllRequests function as an Express RequestHandler so that req, res, and next are no longer any

linux-foundation-easycla · 2020-08-02T09:48:44Z

The committers are authorized under a signed CLA.

✅ Mick Dekkers (ddb07c8, d254fbd)

codecov · 2020-08-02T09:52:11Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1374      +/-   ##
==========================================
- Coverage   93.77%   93.61%   -0.16%     
==========================================
  Files         149      149              
  Lines        4322     4261      -61     
  Branches      880      870      -10     
==========================================
- Hits         4053     3989      -64     
- Misses        269      272       +3

Impacted Files	Coverage Δ
...lemetry-exporter-collector/src/transformMetrics.ts	`80.68% <0.00%> (-3.19%)`	⬇️
...metry-core/src/context/propagation/B3Propagator.ts	`97.67% <0.00%> (-2.33%)`	⬇️
packages/opentelemetry-metrics/src/export/types.ts	`100.00% <0.00%> (ø)`
...pentelemetry-metrics/src/export/aggregators/Sum.ts	`100.00% <0.00%> (ø)`
...emetry-metrics/src/export/aggregators/Histogram.ts	`100.00% <0.00%> (ø)`
...trics/src/export/aggregators/MinMaxLastSumCount.ts	`100.00% <0.00%> (ø)`

mickdekkers · 2020-08-02T19:34:09Z

I noticed 2 more things while I was following the guide:

typescript is not a (dev)dependency, so the npm scripts don't work (well, they might inside the opentelemetry-js repo due to phantom dependency).
the README.md of the guide explains step by step how to arrive at the state in the folder and includes code samples and npm install commands. We'll want to update the readme to keep it in sync.

I'll try to update this PR soon, probably tomorrow.

obecny · 2020-08-03T21:27:05Z

getting-started/ts-example/monitoring.ts

 import { PrometheusExporter } from '@opentelemetry/exporter-prometheus';
+import { RequestHandler } from "express";


interesting is that this line didn't cause the build to fail on linting, as there should be single quotes.

I don't think the getting started guide is linted

@mickdekkers can you please change this line to use single quotes ?

Sure thing, I'll change it to single quotes 👍

FYI, this isn't consistent in the getting started guide: tracing.ts uses both single and double quotes and app.ts uses double quotes

Like I said, the guide isn't linted. Since the guide rarely changes, I think it isn't necessarily a problem. Let's make these small quote changes and merge this. Eventually, the guide will need to be rewritten for the sdk package, and maybe we should start linting it then.

dyladan · 2020-08-05T16:51:41Z

I am going to merge this, with a follow up to lint the getting started guide

…#1374) Co-authored-by: Daniel Dyla <[email protected]>

* feat: graceful shutdown for tracing and metrics * fix: wording in test case * fix: typo * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <[email protected]> * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <[email protected]> * fix: add callbacks to shutdown methods * fix: merge conflict * simplify meter shutdown code Co-authored-by: Daniel Dyla <[email protected]> * fix: fix one-liner * private function name style fix Co-authored-by: Daniel Dyla <[email protected]> * fix: naming of private member variables * fix: graceful shutdown now works in browser * fix: window event listener will trigger once * fix: modify global shutdown helper functions * fix: remove callback from remove listener args * fix: change global shutdown function names and simplify functionality * fix: add rest of function refactoring and simplification * fix: remove unintended code snippet * fix: refactor naming of listener cleanup function and fix sandbox issue * fix: make global shutdown cleanup local * fix: change interval of MeterProvider collection to ensure it does not trigger through clock * chore: removing _cleanupGlobalShutdownListeners * fix: remove unnecesary trace provider member function * Removing default span attributes (#1342) * refactor(opentelemetry-tracing): removing default span attributes Signed-off-by: Aravin Sivakumar <[email protected]> * refactor(opentelemetry-tracing): removing default span attributed from tracer object Signed-off-by: Aravin Sivakumar <[email protected]> * refactor(opentelemetry-tracing): removing accidental add to package.json Signed-off-by: Aravin Sivakumar <[email protected]> * refactor(opentelemetry-tracing): removing redundant test and fixing suggestions by Shawn and Daniel Signed-off-by: Aravin Sivakumar <[email protected]> * feat: add baggage support to the opentracing shim (#918) Co-authored-by: Mayur Kale <[email protected]> * Add nodejs sdk package (#1187) Co-authored-by: Naseem <[email protected]> Co-authored-by: legendecas <[email protected]> Co-authored-by: Mark Wolff <[email protected]> Co-authored-by: Matthew Wear <[email protected]> * feat: add OTEL_LOG_LEVEL env var (#974) * Proto update to latest to support arrays and maps (#1339) * chore: 0.10.0 release proposal (#1345) * fix: add missing grpc-js index (#1358) * chore: 0.10.1 release proposal (#1359) * feat(api/context-base): change compile target to es5 (#1368) * Feat: Make ID generator configurable (#1331) Co-authored-by: Daniel Dyla <[email protected]> * fix: require grpc-js instead of grpc in grpc-js example (#1364) Co-authored-by: Bartlomiej Obecny <[email protected]> * chore(deps): update all non-major dependencies (#1371) * chore: bump metapackage dependencies (#1383) * chore: 0.10.2 proposal (#1382) * fix: remove unnecesary trace provider member function * refactor(metrics): distinguish different aggregator types (#1325) Co-authored-by: Daniel Dyla <[email protected]> * Propagate b3 parentspanid and debug flag (#1346) * feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary (#1320) Co-authored-by: Daniel Dyla <[email protected]> * feat: Collector Metric Exporter for the Web (#1308) Co-authored-by: Daniel Dyla <[email protected]> * Fix issues in TypeScript getting started example code (#1374) Co-authored-by: Daniel Dyla <[email protected]> * chore: deploy canary releases (#1384) * fix: protos pull * fix: address marius' feedback * chore: deleting removeAllListeners from prometheus, fixing tests, cleanu of events when using shutdown notifier * fix: add documentation and cleanup code * fix: remove async label from shutdown and cleanup test case * fix: update controller collect to return promise * fix: make downsides of disabling graceful shutdown more apparent Co-authored-by: Daniel Dyla <[email protected]> Co-authored-by: Bartlomiej Obecny <[email protected]> Co-authored-by: Aravin <[email protected]> Co-authored-by: Ruben Vargas Palma <[email protected]> Co-authored-by: Mayur Kale <[email protected]> Co-authored-by: Naseem <[email protected]> Co-authored-by: legendecas <[email protected]> Co-authored-by: Mark Wolff <[email protected]> Co-authored-by: Matthew Wear <[email protected]> Co-authored-by: Naseem <[email protected]> Co-authored-by: Mark Wolff <[email protected]> Co-authored-by: Cong Zou <[email protected]> Co-authored-by: Reginald McDonald <[email protected]> Co-authored-by: WhiteSource Renovate <[email protected]> Co-authored-by: srjames90 <[email protected]> Co-authored-by: David W <[email protected]> Co-authored-by: Mick Dekkers <[email protected]>

* chore(mongodb): fix build after v5 release * chore: pin @types/bson version in monorepo root for lerna hoisting * chore: pin @types/bson version in monorepo root for lerna hoisting * chore: pin @types/bson version in mongodb * chore: patch winston module (logform breaking), ts 4.5.5 fixes * chore: change typescript to 4.4.4 * chore: change typescript to 4.4.4

mickdekkers added 2 commits August 2, 2020 11:45

fix(getting-started): match return type of createCounter

ddb07c8

fix(getting-started): close gaps in TypeScript type coverage

d254fbd

mickdekkers requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners August 2, 2020 09:48

naseemkullah approved these changes Aug 2, 2020

View reviewed changes

mayurkale22 approved these changes Aug 2, 2020

View reviewed changes

vmarchaud approved these changes Aug 2, 2020

View reviewed changes

obecny reviewed Aug 3, 2020

View reviewed changes

dyladan approved these changes Aug 3, 2020

View reviewed changes

Merge branch 'master' into patch-1

9acc472

dyladan mentioned this pull request Aug 5, 2020

Lint getting started guide #1390

Closed

dyladan merged commit 88cd842 into open-telemetry:master Aug 5, 2020

jonahrosenblum pushed a commit to jonahrosenblum/opentelemetry-js that referenced this pull request Aug 8, 2020

Fix issues in TypeScript getting started example code (open-telemetry…

70bee58

…#1374) Co-authored-by: Daniel Dyla <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues in TypeScript getting started example code #1374

Fix issues in TypeScript getting started example code #1374

mickdekkers commented Aug 2, 2020

linux-foundation-easycla bot commented Aug 2, 2020 •

edited

Loading

codecov bot commented Aug 2, 2020 •

edited

Loading

mickdekkers commented Aug 2, 2020

obecny Aug 3, 2020

dyladan Aug 3, 2020

obecny Aug 3, 2020

mickdekkers Aug 4, 2020

dyladan Aug 4, 2020

dyladan commented Aug 5, 2020

		import { PrometheusExporter } from '@opentelemetry/exporter-prometheus';
		import { RequestHandler } from "express";

Fix issues in TypeScript getting started example code #1374

Fix issues in TypeScript getting started example code #1374

Conversation

mickdekkers commented Aug 2, 2020

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Aug 2, 2020 • edited Loading

codecov bot commented Aug 2, 2020 • edited Loading

Codecov Report

mickdekkers commented Aug 2, 2020

obecny Aug 3, 2020

Choose a reason for hiding this comment

dyladan Aug 3, 2020

Choose a reason for hiding this comment

obecny Aug 3, 2020

Choose a reason for hiding this comment

mickdekkers Aug 4, 2020

Choose a reason for hiding this comment

dyladan Aug 4, 2020

Choose a reason for hiding this comment

dyladan commented Aug 5, 2020

linux-foundation-easycla bot commented Aug 2, 2020 •

edited

Loading

codecov bot commented Aug 2, 2020 •

edited

Loading