Use HTTP GetSettings #527
Use HTTP GetSettings #527
Conversation
| import { hmac } from "@noble/hashes/hmac" | ||
| import { sha1 } from "@noble/hashes/sha1" |
There was a problem hiding this comment.
Pure JavaScript implementations that work in the browser. noble is routinely audited and probably the best regarded crypto implementation for JavaScript.
| @@ -68,7 +68,7 @@ export class AppopticsInboundMetricsProcessor | |||
| method: meta.method, | |||
| status: meta.status, | |||
| url: meta.url, | |||
| domain: meta.hostname, | |||
| domain: null, | |||
There was a problem hiding this comment.
This produced unexpected transaction names.
| grpc_proxy: config.proxy ?? "", | ||
| reporter: "ssl", | ||
| metric_format: 1, | ||
| metric_format: config.appoptics ? 1 : 2, |
There was a problem hiding this comment.
Make legacy mode better compatible with SWO.
| const collector = z.string().transform((c, ctx) => { | ||
| if (!/^https?:/.test(c)) { | ||
| c = `https://${c}` | ||
| } | ||
| try { | ||
| return fs.readFile(p, "utf-8") | ||
| return new URL(c) | ||
| } catch (err) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: (err as Error).message, | ||
| }) | ||
| return z.NEVER | ||
| } | ||
| }) |
There was a problem hiding this comment.
Parse the collector directly to a URL object, which will catch some errors early if it's set to a nonsensical value.
There was a problem hiding this comment.
It looks like using proper variable names would help with readability and providing context here?
| headers: Record<string, string> | ||
| otlp: { | ||
| tracesEndpoint?: string | ||
| metricsEndpoint?: string | ||
| logsEndpoint?: string | ||
| headers: Record<string, string> | ||
| } |
There was a problem hiding this comment.
headers field moved out of otlp since they're now also used by the sampler !
raphael-theriault-swi
force-pushed
the
NH-98076
branch
from
d66b31f to
8042223
Compare
| const collector = z.string().transform((c, ctx) => { | ||
| if (!/^https?:/.test(c)) { | ||
| c = `https://${c}` | ||
| } | ||
| try { | ||
| return fs.readFile(p, "utf-8") | ||
| return new URL(c) | ||
| } catch (err) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: (err as Error).message, | ||
| }) | ||
| return z.NEVER | ||
| } | ||
| }) |
There was a problem hiding this comment.
It looks like using proper variable names would help with readability and providing context here?
| .replace(/^apm\.collector\./, "https://otel.collector.") | ||
| .concat(ENDPOINTS.traces), | ||
| metricsEndpoint: | ||
| otel.OTEL_EXPORTER_OTLP_METRICS_ENDPOINT ?? | ||
| otel.OTEL_EXPORTER_OTLP_ENDPOINT?.concat(ENDPOINTS.metrics) ?? | ||
| raw.collector | ||
| raw.collector.hostname | ||
| .replace(/^apm\.collector\./, "https://otel.collector.") |
There was a problem hiding this comment.
very clever! however, it looks like https://apm.collector... would be skipped?
| } | ||
|
|
||
| /** Settings update loop */ | ||
| #loop() { |
There was a problem hiding this comment.
it looks like this would be a nice use case for web workers in browser?
There was a problem hiding this comment.
I do wish OTel actually offloaded most processing to a worker but as it is most of the code needs to run on the main worker and having just this loop run in a separate worker and communicate with the main one would add a lot of complexity for not much of a payoff.
raphael-theriault-swi
force-pushed
the
NH-98076
branch
from
8042223 to
624b40b
Compare
This switches from gRPC to HTTP as the protocol to obtain sampling settings from the collector. More logic can be shared with the Lambda JSON sampler, and we drop all dependencies on gRPC and the apm-proto package. Some changes were also made to allow running the sampler in a browser context.