Exporter packages #115

arendjr · 2023-08-30T19:31:03Z

This turned out to be much more than I thought, so I haven't actually gotten to testing yet. Still, I think it's due for a first round of review.

Summary

(Detailed explanations below.)

Split all the exporters into separate packages.
Dropped the autometrics convenience package.
Rewrote the logic for push exporters.
Implemented OTLP/HTTP exporter.
Enabled strict settings in TypeScript.
Switched from NPM to Yarn.
Updated all the examples and READMEs.
Fixes 🐛 Class decorators break in the browser when using the init function #113
Resolves OTLP export support #114

Checklist

Tested with OTLP connector
Tested with Gravel gateway
Deprecated autometrics package.
Published @autometrics/on-demand-metric-reader.
Created test case for 🐛 Class decorators break in the browser when using the init function #113.

Details

Split all the exporters into separate packages

Since I would be adding another exporter, and different exporters rely on different packages, it only made sense to split the exporters themselves into packages as well.

This also resolved the issue where we decided we don't want to automatically start a webserver anymore. This behavior is now explicitly limited to the Prometheus exporter only, and clearly documented as such.

Dropped the `autometrics` convenience package

Given that users now need to make an explicit choice regarding the exporter to use, the convenience package didn't make much sense anymore.

Rewrote the logic for push exporters

I initially tried to implement my new OTLP/HTTP exporter on the existing push gateway support, but I slowly discovered that we were misusing the Otel APIs. In order to fix it "the right way", I had to implement a new OnDemandMetricReader (in a separate package for now, but I think it makes sense to upstream this to the OpenTelemetry-js project) and a PushGatewayExporter (could maybe be upstreamed to as a contrib package? I dunno). Everything feels horribly over-engineered, but that seems to be the way of the Otel project.

Since I was copying/reusing some bits from the Otel libraries anyway, I brought in a few more options that wouldn't hurt to have either, such as timeout and concurrencyLimit.

Implemented OTLP/HTTP exporter

That's what it all started with. I'll soon test if it actually works 😅

Enabled `strict` settings in TypeScript

While I would generally advise anyone using TS to enable the strict settings, this is especially true for libraries. Otherwise consumers with strict settings may run into trouble consuming our APIs since we didn't strict-check them.

Switched from NPM to Yarn

After running into another NPM issue with unnecessarily duplicated packages and NPM's failure to install Rome I was fed up and decided to swap it out for Yarn. All our other TS projects use Yarn anyway.

I quickly realized there's another important advantage for libraries, since Yarn actually warns when peer dependencies aren't specified correctly and is generally more strict with how dependencies are specified. Similar to the strict settings, if we don't verify our packages are specified correctly, downstream Yarn (and PNPM) consumers may run into issues instead.

Updated all the examples and READMEs

Since there's quite some breaking changes in the API, I had to run through everything.

Please double-check I didn't miss anything.

Fixes #113

Given that the init() functions now come from separate packages anyway, I had to make things a bit more lenient to avoid a big footgun here. So now we always create a MeterProvider immediately, so metrics recording can work before the exporter is even registered. The situation of accidentally spinning up the webserver for the Prometheus scrape endpoint is simply impossible now because that server is in a separate package.

Resolves #114

I think this resolves everything I set out to do, and more :)

brettimus

approving but i haven't done any e2e tests yet! would still like to try that before merging

brettimus · 2023-09-04T14:37:26Z

examples/react-app/README.md

@@ -0,0 +1,68 @@
+## Example React App set up with Vite.js and configured with Autometrics


might be good to highlight the experimental nature of doing something like this! since we haven't worked out all the kinks oursevles

brettimus · 2023-09-04T14:47:06Z

packages/autometrics/README.md

+import { autometrics } from "@autometrics/autometrics";
+import { init } from "@autometrics/exporter-otlp-http";
+
+init({ url: "https://<your-otel-collector>" });


same comment as above: i think this was a copy-paste from the otel readme. not sure if we want to include init as an example of generic usage

How do you mean, "want to include [...] as an example of generic usage"? Calling init() is mandatory now.

brettimus · 2023-09-04T14:47:30Z

packages/autometrics/package.json

  "contributors": [
    "Laurynas Keturakis",
    "Evan Schwartz",
    "Jacco Flenter",
    "Oscar van Zijverden",
-    "Stephan Lagerwaard"
+    "Stephan Lagerwaard",
+    "Arend van Beelen jr."


brettimus · 2023-09-04T15:13:51Z

packages/exporter-otlp-http/src/index.ts

+  timeout = 1000,
+  buildInfo,
+}: InitOptions) {
+  amLogger.info(`Exporter will push to the OLTP/HTTP endpoint at ${url}`);


nit: OTLP 🙈

brettimus · 2023-09-04T15:28:48Z

packages/exporter-prometheus-push-gateway/src/pushGatewayExporter.ts

+  private _export(serializedMetrics: string): Promise<void> {
+    const promise = fetch(this._options.url, {
+      method: "POST",
+      mode: "cors",


note to self: will "cors" have an effect in node? i remember getting typescript complaints from using this in a node app once

Good question! I did a little search, at least node-fetch says they haven't implemented it (https://www.npmjs.com/package/node-fetch). I would expect them to ignore it, but I can't know for sure without testing. Of course, Node has native fetch() nowadays, but MDN makes no mention of Node's support for the mode property: https://developer.mozilla.org/en-US/docs/Web/API/Request/mode
It does say Deno doesn't support it, but just like with node-fetch, I would assume it gets ignored...

brettimus · 2023-09-04T16:19:14Z

README.md

+import { autometrics } from "@autometrics/autometrics";
+import { init } from "@autometrics/exporter-otlp-http";
+
+init({ url: "https://<your-otel-collector>" });


i think this was a copy-paste from the otel readme. not sure if we want to include init as an example of generic usage

Copy-paste?! I'm no AI 🤣

brettimus · 2023-09-04T16:20:04Z

examples/react-app/src/App.tsx

-  pushGateway: "http://0.0.0.0:8080/metrics",
+  // A proxy configured in `vite.config.ts` forwards the `/metrics` endpoint
+  // to the Otel Collector.
+  url: "https://wercker.fmp.fiberplane.dev/otel/http/v1/metrics",


we'll probably want to change this example URL before merging

Oh, that was an accidental check-in because Benno asked me to test something. I'll revert that :D

arendjr added 8 commits August 28, 2023 19:47

WIP: Split exporters into separate packages

aa6243e

WIP

381f142

Update examples

a5a082b

WIP

2e83ecd

Fix TS builds

5496f4f

WIP

1f682b2

Finish things up

528e631

Simplification

47aa4e6

arendjr requested review from keturiosakys and brettimus August 30, 2023 19:31

arendjr added 3 commits August 30, 2023 21:33

Remove SDKs

de21b61

Most things seem to work again

36e3e81

Fix tests

e420d20

arendjr force-pushed the exporter-packages branch from 64355ca to d27aae3 Compare September 1, 2023 12:33

Fix lock file

7633563

arendjr force-pushed the exporter-packages branch from d27aae3 to 7633563 Compare September 1, 2023 12:40

arendjr added 2 commits September 1, 2023 14:47

CI fix

9bdc988

React example works

ef555b9

arendjr force-pushed the exporter-packages branch from 5c3f1a7 to d4de8af Compare September 4, 2023 09:34

Add README for React example

a984b82

arendjr force-pushed the exporter-packages branch from d4de8af to a984b82 Compare September 4, 2023 09:36

Improved docs

46af537

arendjr force-pushed the exporter-packages branch from d550697 to 46af537 Compare September 4, 2023 13:21

Fix an incomplete comment I wrote awhile ago

83c494f

brettimus approved these changes Sep 4, 2023

View reviewed changes

arendjr added 2 commits September 5, 2023 10:42

PR feedback

89a5606

Make it clear React app example is experimental

b85cf02

arendjr force-pushed the exporter-packages branch from a834977 to b85cf02 Compare September 5, 2023 08:48

arendjr added 2 commits September 5, 2023 13:12

Add integration test for OTLP -- still failing

56a22ec

Fix edge case with metrics being recorded before init()

ccfaa02

arendjr added 2 commits September 6, 2023 10:20

Remove copied comment

745bda3

Fix temporary meter

9296f10

arendjr force-pushed the exporter-packages branch from 442d180 to 9296f10 Compare September 6, 2023 08:24

arendjr merged commit 90400cd into main Sep 6, 2023
1 check passed

arendjr deleted the exporter-packages branch September 6, 2023 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exporter packages #115

Exporter packages #115

arendjr commented Aug 30, 2023 •

edited

Loading

brettimus left a comment

brettimus Sep 4, 2023

brettimus Sep 4, 2023

arendjr Sep 5, 2023

brettimus Sep 4, 2023

brettimus Sep 4, 2023

brettimus Sep 4, 2023

arendjr Sep 5, 2023

brettimus Sep 4, 2023

arendjr Sep 5, 2023

brettimus Sep 4, 2023

arendjr Sep 5, 2023

		@@ -0,0 +1,68 @@
		## Example React App set up with Vite.js and configured with Autometrics

Exporter packages #115

Exporter packages #115

Conversation

arendjr commented Aug 30, 2023 • edited Loading

Summary

Checklist

Details

Split all the exporters into separate packages

Dropped the autometrics convenience package

Rewrote the logic for push exporters

Implemented OTLP/HTTP exporter

Enabled strict settings in TypeScript

Switched from NPM to Yarn

Updated all the examples and READMEs

Fixes #113

Resolves #114

brettimus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arendjr commented Aug 30, 2023 •

edited

Loading

Dropped the `autometrics` convenience package

Enabled `strict` settings in TypeScript