Add tracing #8
Add tracing #8
Conversation
package.json
Outdated
| @@ -35,31 +35,33 @@ | |||
| "git-hooks:pre-commit": "lint-staged", | |||
| "drone": "run-s drone:lint drone:sign", | |||
| "drone:lint": "drone lint .drone/drone.yml --trusted", | |||
| "drone:sign": "drone --server https://drone.grafana.net sign --save grafana/grafana-javascript-agent .drone/drone.yml" | |||
| "drone:sign": "drone --server https://drone.grafana.net sign --save grafana/grafana-javascript-agent .drone/drone.yml", | |||
| "postinstall": "npm explore @opentelemetry/otlp-exporter-base -- cp -r build/src build/esm" | |||
There was a problem hiding this comment.
Let's use cpy instead of cp as people may have issues on Windows machines.
There was a problem hiding this comment.
cpy CLI package: https://www.npmjs.com/package/cpy-cli
There was a problem hiding this comment.
It seems a new version was released that fixes the problem being sovled here hopefully, maybe we can get rid of this hook!
check new version https://www.npmjs.com/package/@opentelemetry/otlp-exporter-base
There was a problem hiding this comment.
New version breaks payload compat with version of otel collector used by grafana agent, and still has a build issue: open-telemetry/opentelemetry-js#2998
There was a problem hiding this comment.
cpy-cli does not work like cp, ends up with an odd dir hierarchy. Ended up using shx: https://www.npmjs.com/package/shx
| @@ -15,7 +15,7 @@ | |||
| "scripts": { | |||
| "start": "parcel serve", | |||
| "build": "parcel build", | |||
| "clean": "rimraf dist/", | |||
| "clean": "rimraf dist/ yarn-error.log", | |||
There was a problem hiding this comment.
Is yarn-error.log something that has to be removed? Originally clean was responsible with cleaning the build caches rather than logs.
There was a problem hiding this comment.
This change was made by you :) Shall I remove it?
| @@ -1,42 +1,59 @@ | |||
| function throwError() { | |||
| import '@grafana/agent-web/dist/globals'; | |||
There was a problem hiding this comment.
Should we export this through a barrel rather than importing it like this?
There was a problem hiding this comment.
I wanted extending a global type to be explicit choice rather than happening automatically by importing from a barrel.
Two reasons:
- It might be wrong if you renamed the global from "grafanaAgent" via config
- Extending global types can be contraversial
|
|
||
| // trace context for logs, exceptions, measurements | ||
| export interface TraceContext { | ||
| trace_id: string; |
There was a problem hiding this comment.
Should we move to camelcase instead of snakecase?
There was a problem hiding this comment.
no, this goes straight to json where it should be sanke cased. Unless we want to rewrite it later?
There was a problem hiding this comment.
actually I'm not sure that go unmarshaller won't auto convert 🤔 will test later
|
|
||
| const getTraceContext = (): TraceContext | undefined => { | ||
| if (otel) { | ||
| const ctx = otel.trace.getSpanContext(otel.context.active()); |
There was a problem hiding this comment.
Should we create some helpers for OTel packages? I see a need for getCurrentContext and getCurrentSpan
There was a problem hiding this comment.
I feel we should just use otel api and avoid adding any layers on top of it. This is less confusing (one way to do things), and I think we don't really have enough experience to be opinionated about it
Co-authored-by: Bogdan Matei <bogdan.matei@grafana.com>
* add simplified web initializer * re-disable LogLevel.LOG for console instrumentation * getDefaultInstrumentations -> getWebInstrumentations
fixes #10