feat: ✨ Endpoint & API Key config #22

pkanal · 2024-01-05T16:56:58Z

Which problem is this PR solving?

Closes Add Honeycomb config options #9

Short description of the changes

Add Honeycomb specific config option types
Wire up endpoint and api key config

Out of scope(for this PR):

Baggage Span Processor
Resource attributes
Sample rate
Debug mode
Local visualizations
Environment variable support

How to verify that this has the expected result

Run the example app with an API key and see traces in Honeycomb

wnstn · 2024-01-08T16:00:38Z

src/types.ts

+  serviceName?: string;
+
+  /** The sample rate used to determine whether a trace is exported. Defaults to 1 (send everything). */
+  sampleRate?: number;


This is at a trace level, not a session level, right?

Yep this is trace level!

tell people what to set it to for something else. I think it's a whole number, "send 1 of every sampleRate traces"

wnstn · 2024-01-08T16:02:31Z

src/util.ts

+ * @param apikey the apikey
+ * @returns a boolean to indicate if the apikey was a classic key
+ */
+export function isClassic(apikey?: string): boolean {


just a note, this will soon have a third variation, not sure if you want to go ahead and prepare for that (talk to API&Partnerships for the doc around new key structure)

This is a good point, I was thinking about this and I believe this check still stands because we're checking if a key is Classic or not and Classic keys aren't changing from 32 characters. This would only be affected if any newer keys can be 32 characters which I don't think is the case but I will double check with API&Partnerships on that!

All our code uses this "isClassic" implementation.

kentquirk

I added some comments, but feel free to ignore them if I'm missing the point.

kentquirk · 2024-01-08T16:32:00Z

src/util.ts

+ * based exporter protocol.
+ *
+ * @param url the base URL to append traces path to if missing
+ * @param protocol the exporter protocol to send telemetry


this parameter isn't on the function

kentquirk · 2024-01-08T16:32:11Z

examples/hello-world-web/index.js


 const main = () => {
  // Set OTel to log in Debug mode
  diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG);

  // Initialize base OTel WebSDK
-  const sdk = new WebSDK({
+  const sdk = new HoneycombWebSDK({
+    endpoint: 'https://api.honeycomb.io/v1/traces',


Since you have code below to add /v1/traces if it's missing, maybe it would be better to leave it off?

kentquirk · 2024-01-08T16:35:16Z

src/util.ts

+ * @param protocol the exporter protocol to send telemetry
+ * @returns the endpoint with traces path appended if missing
+ */
+export function maybeAppendTracesPath(url: string) {


I think we might have a situation where if they add a trailing slash, it doesn't work. If so, should we check for that?

Also, what about logs?

Great catch! Updated the logic and I'll make a PR to the Node distro to update it there as well 👍🏽

As for logs, the distro doesn't have a log exporter at the moment so there will be a separate function and exporter for that when we support it.

Sadly, logs are not considered stable in otel-js yet.
We're going to need them for FEO, but maybe not for RUM

kentquirk · 2024-01-08T16:37:31Z

test/util.test.ts

+
+// classic keys are 32 chars long
+const classicApiKey = 'this is a string that is 32 char';
+// non-classic keys are 22 chars log


technically, because they're not 0-padded, they can be any length up to 23, IIRC. But we should never depend on it.

Also, classic keys are hex only, while non-classic are alphanumeric.

test/util.test.ts

kentquirk · 2024-01-08T16:38:28Z

test/util.test.ts

+    expect(endpoint).toBe('https://api.honeycomb.io/v1/traces');
+  });
+
+  it('does not append the traces path if the url ends with /v1/traces', () => {


add a test for if they give you /v1/traces/?

Co-authored-by: Kent Quirk <kentquirk@gmail.com>

jessitron · 2024-01-08T22:01:48Z

I don't see any changes

Meaning: there's nothing new in the README, so from an external perspective this project is the same 😛

kentquirk

LGTM!

jessitron · 2024-01-08T22:06:44Z

src/http-json-trace-exporter.ts

+    url: getTracesEndpoint(options),
+    headers: {
+      [TEAM_HEADER_KEY]: apiKey,
+      [DATASET_HEADER_KEY]: isClassic(apiKey) ? options?.dataset : undefined,


dataset is different from serviceName? we don't fall back to serviceName? We don't fail if it's classic and dataset is unpopulated?
... the send will fail, which is hard to diagnose

(my surprise will be cured by documentation)

Yes, there's no dataset header needed fpr ingest needed for non-classic. It just uses whatever service.name is set to, and the backend assigns a dataset for you.

@cartermp I was thinking that maybe it makes sense to default dataset to service.name in classic? ... but that's probably wrong too, because it would have to be coincidentally correct.

I do think it makes sense to fail when we don't get a dataset header in Classic mode, so we can give them a link to the page that explains it.

jessitron · 2024-01-08T22:07:34Z

src/types.ts

+  /** The API key used to send telemetry to Honeycomb. */
+  apiKey?: string;
+
+  /** The API key used to send traces telemetry to Honeycomb. Defaults to apikey if not set. */


as opposed to?
... metrics maybe?

hmm, no, I don't see a metrics one. so what is this one for?

We don't support metrics yet but this is there because if/when we do there will also be metricsApiKey and logsApiKey

jessitron · 2024-01-08T22:08:58Z

src/honeycomb-otel-sdk.ts

+export class HoneycombWebSDK extends WebSDK {
+  constructor(options?: HoneycombOptions) {
+    super({
+      spanProcessor: new BatchSpanProcessor(


right here at this line I say, "oh, I'm not gonna use this."

Because I can't override the spanProcessor to send to my local collector when I'm ready to do that.

Hmmm we're going to struggle with this because we want to provide our own BrowserAttributesSpanProcessor so this can be changed to passing directly to a trace exporter but eventually we'll have to use the spanProcessor attribute.

I wonder if there's a way we can accept a span processor that we extend with the things we want to add to it 🤔 . For now I'll change this to exporting through the traceExporter option.

yes, accepting a span processor and wrapping it is a good idea!

jessitron · 2024-01-08T22:10:18Z

src/types.ts

+  dataset?: string;
+
+  /** The service name of the application and where traces telemetry is stored in Honeycomb. */
+  serviceName?: string;


defaults to "unknown_service"

jessitron · 2024-01-08T22:10:49Z

src/types.ts

+  /** The API endpoint where traces telemetry is sent. Defaults to endpoint if not set. */
+  tracesEndpoint?: string;
+
+  /** The dataset where traces telemetry is stored in Honeycomb. Only used when using a classic API key. */


and required when using a classic API key, link to https://docs.honeycomb.io/honeycomb-classic/#am-i-using-honeycomb-classic

jessitron · 2024-01-08T22:11:05Z

src/types.ts

+  /** The debug flag enables additional logging that us useful when debugging your application. Do not use in production. */
+  debug?: boolean;
+
+  /** The local visualizations flag enables logging Honeycomb URLs for completed traces. Do not use in production. */


this one is my FAVORITE

jessitron · 2024-01-08T22:11:42Z

src/types.ts

+   * used in conjuction with an OpenTelemetry Collector (which will handle the API key and dataset configuration).
+   * Defaults to 'false'.
+   */
+  skipOptionsValidation?: boolean;


what if we made an option for "I have a local collector"

I like this idea, right now all our other distros are structured this way so I think we would have to have a larger conversation about adding that option

it's OK for this distro to be ahead of the others, but it does make sense to talk to the other maintainers about this direction

jessitron · 2024-01-08T22:13:17Z

test/honeycomb-otel-sdk.test.ts

  const honeycomb = new HoneycombWebSDK();
-  expect(honeycomb).toBeDefined();
+  expect(honeycomb).toBeInstanceOf(WebSDK);
 });


... if you can pass in options of a span processor, then you can pass in a test spanprocessor and do more testing.

(another vote for passing in a span processor)

jessitron · 2024-01-08T22:15:18Z

src/types.ts

+  /** The API endpoint where telemetry is sent. Defaults to 'https://api.honeycomb.io' */
+  endpoint?: string;
+
+  /** The API endpoint where traces telemetry is sent. Defaults to endpoint if not set. */


It defaults to endpoint/v1/traces !!! That is why you would set this endpoint

pkanal · 2024-01-10T15:48:06Z

README.md

+| *serviceName* | optional | string|unknown_service | The name of this browser application. Your telemetry will go to a Honeycomb dataset with this name. |
+| *localVisualizations*| optional | boolean | false | For each trace created, print a link to the console so that you can find it in Honeycomb. Super useful in development! Do not use in production. |
+| sampleRate | optional | number |1 | If you want to send a random fraction of traces, then make this a whole number greater than 1. Only 1 in `sampleRate` traces will be sent, and the rest never be created. |
+| tracesEndpoint | optional | string|`${endpoint}/v1/traces` | Populate this to send traces to a route other than /v1/traces |


This actually doesn't append /v1/traces to an endpoint. This option exists similar to tracesApiKey for when you want to set different endpoints for different signals.

if you don't set tracesEndpoint, then does it send to $endpoint/v1/traces?

... because that's what the regular SDK does

hmm so are we saying, we default tracesEndpoint to $endpoint, but then Otel adds /v1/traces under the hood if it's only a domain?

The logic we follow for endpoints is laid out in our distro spec.

This is the order of logic:

if tracesEndpoint is set, use that and do not append /v1/traces

if tracesEndpoint is not set, fall back to endpoint which will append /v1/traces

if endpoint is not set, default to https://api.honeycomb.io with /v1/traces appended

We add /v1/traces to endpoint automatically (OTel doesn't do this under the hood), as long as the endpointstring doesn't already end in/v1/traces`.

pkanal · 2024-01-19T16:44:45Z

Merging this for now, will have follow up PRs for tests & collector option

pkanal added 5 commits January 4, 2024 13:37

very basic configuration for apikey and endpoint

3074a94

configure json exporter

149fae5

util function tests

dba51e3

add apiKey option logic

11c622e

Node --> Web

e5199bc

pkanal changed the title ~~Purvi/basic-config~~ feat: ✨ Endpoint & API Key config Jan 5, 2024

pkanal requested review from a team and jessitron January 5, 2024 19:00

pkanal self-assigned this Jan 5, 2024

pkanal added the type: enhancement New feature or request label Jan 5, 2024

pkanal added this to the Alpha milestone Jan 5, 2024

pkanal marked this pull request as ready for review January 5, 2024 19:01

pkanal requested a review from a team as a code owner January 5, 2024 19:01

fix lint

9489078

wnstn reviewed Jan 8, 2024

View reviewed changes

kentquirk reviewed Jan 8, 2024

View reviewed changes

pkanal and others added 5 commits January 8, 2024 13:25

Update test/util.test.ts

6adcda4

Co-authored-by: Kent Quirk <kentquirk@gmail.com>

address PR comments

b51a44b

fix the trailing slash bug

c450de4

update test name

4c71e00

remove endpoint option from example

35e6d20

kentquirk approved these changes Jan 8, 2024

View reviewed changes

jessitron reviewed Jan 8, 2024

View reviewed changes

jessitron added 2 commits January 9, 2024 17:05

Populate the README with some how-to and the configuration

01f929d

docs: oops this sentence does not belong

d4e44b8

pkanal commented Jan 10, 2024

View reviewed changes

pkanal added 3 commits January 18, 2024 10:37

update docstrings

3c8f638

switch to traceExporter

140c84f

fix lint

e8108c7

pkanal merged commit 40c3682 into main Jan 19, 2024
7 checks passed

pkanal deleted the purvi/basic-config branch January 19, 2024 16:44

feat: ✨ Endpoint & API Key config #22

feat: ✨ Endpoint & API Key config #22

Conversation

pkanal commented Jan 5, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

How to verify that this has the expected result

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

kentquirk 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

jessitron commented Jan 8, 2024

kentquirk 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

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

jessitron Jan 8, 2024 • edited Loading

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

pkanal commented Jan 19, 2024

pkanal commented Jan 5, 2024 •

edited

Loading

jessitron Jan 8, 2024 •

edited

Loading