Repository: honeycombio/beeline-nodejs (js)

Issue:
Update for E&S
**Is your feature request related to a problem? Please describe.**

Environments & Services are coming to Honeycomb. The beeline should be updated to support sending data to environments while maintaining backward compatibility. To achieve this, we will set dataset from service name, and provide appropriate warnings depending on environment vs. non-environment teams.

**Describe the solution you'd like**

Tracing scenario for environment-aware teams:

* Dataset should no longer propagate across services for new environment key.
* `ServiceName` "required"†. If unset, will warn and set `service_name` AND `service.name` to `unknown_service:<process>` or `unknown_service:language`. Service name is used to populate dataset name; if `unknown_service.*` then truncate to`unknown_service` for dataset name. `service.name` field will be added to the already added `service_name` for closer consistency with OTel.
* `WriteKey` required. If unset, will warn.
* `Dataset` **ignored**. If unset, no warning (because no longer used). If set, will warn to clarify that it will be ignored in favor of service name.

Tracing scenario for legacy (non-environment-aware) teams:

* `ServiceName` "required"†. If unset, will warn.
* `WriteKey` required. If unset, will warn.
* `Dataset` required. If unset, will warn and default to `beeline-<language>`.

† While not technically required, service name is highly encouraged to avoid `unknown_service` and to instead properly describe the data being sent to Honeycomb (and the data being viewed in Honeycomb UI).

This will be addressed in a separate issue/PR: Set [spanKind value based on OTel spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#spankind) for consistency across beelines and OTel.

### Notes from decision around defaulting when no service name is provided:
*Given* a beeline configured with a v2 api key and without a service name
*When* it sends HNY events
*Then* it should use `unknown_service` as the dataset
*And* it should use `unknown_service:process-name` as service_name

When given a legacy key, the behavior remains the same as today.

[Notes on trimming whitespace](https://honeycomb.quip.com/qdrvA6a9AHEI/notes-trimming-whitespace-ingest)

Pull request description:
Add Environment & Services support
<!--
Thank you for contributing to the project! 💜
Please make sure to:
- Chat with us first if this is a big change
  - Open a new issue (or comment on an existing one)
  - We want to make sure you don't spend time implementing something we might have to say No to
- Add unit tests
- Mention any relevant issues in the PR description (e.g. "Fixes #123")

Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate.
-->

## Which problem is this PR solving?
Adds E&S support by ensuring service name and dataset are configured correctly for both classic and non-classic API keys. Classic keys use the dataset and non-classic uses the servicename for target dataset.

The beeline also now assigns a default service name based on the API key provided. Classic gets "nodejs" and non-classic gets "unknown_service:{process-name}" where process name is the node process.name name if assigned and falls back to "nodejs" is it cannot resolve one.

The Beeline now applies the following defaults:
| Property | Classic | Non-classic |
| - | - | - |
| service name | unknown_service:{process.name\|nodejs} | unknown_service:{process.name\|nodejs} |
| dataset | nodejs | unknown_service |

Note, I selected to not update the default dataset from `nodejs` to `beeline-nodejs` as this would be a break change for anyone relying on that now. We can make the change in the next major release.

- Closes #550 

## Short description of the changes
- Update libhoney.configure to detect and apply defaults for service name and dataset
- Warn on missing API key, dataset (classic) and service name (non-classic)
- Update tests and add extra tests for default service name and dataset
- Don't propagate dataset in Honeycomb propagation implementation (w3c doesn't propagate dataset field)

Patch:
diff --git a/lib/api/index.js b/lib/api/index.js
index c09a921..db26cbe 100644
--- a/lib/api/index.js
+++ b/lib/api/index.js
@@ -6,7 +6,8 @@ const process = require("process"),
   pkg = require("../../package.json"),
   debug = require("debug")(`${pkg.name}:event`),
   LibhoneyImpl = require("./libhoney"),
-  MockImpl = require("./mock");
+  MockImpl = require("./mock"),
+  utils = require("../util");
 
 const { honeycomb, aws, w3c, util } = propagation;
 
@@ -38,6 +39,9 @@ module.exports = {
     }
     debug(`using impl: ${impl == LibhoneyImpl ? "libhoney-event" : "mock"}`);
     apiImpl = new impl(opts);
+
+    // tell honeycomb propagator whether we should propagate dataset
+    honeycomb.setPropagateDataset(utils.isClassic(opts.writeKey));
   },
 
   traceActive() {
diff --git a/lib/api/libhoney.js b/lib/api/libhoney.js
index 01e1912..78620c9 100644
--- a/lib/api/libhoney.js
+++ b/lib/api/libhoney.js
@@ -10,7 +10,8 @@ const libhoney = require("libhoney"),
   schema = require("../schema"),
   Span = require("./span"),
   pkg = require("../../package.json"),
-  debug = require("debug")(`${pkg.name}:event`);
+  debug = require("debug")(`${pkg.name}:event`),
+  util = require("../util");
 
 const defaultName = "nodejs";
 
@@ -81,7 +82,38 @@ module.exports = class LibhoneyEventAPI {
       debug(`using proxy ${proxy}`);
     }
 
-    this.defaultDataset = opts.dataset || process.env["HONEYCOMB_DATASET"] || defaultName;
+    if (!opts.serviceName) {
+      opts.serviceName = ["unknown_service", process.name ? process.name.trim() : "nodejs"].join(":");
+      console.warn(`empty serviceName configuration option - setting service name to '${opts.serviceName}'`);
+    }
+
+    if (!opts.writeKey) {
+      console.warn("empty writeKey configuration option");
+    }
+
+    if (util.isClassic(opts.writeKey)) {
+      let dataset = opts.dataset || process.env["HONEYCOMB_DATASET"];
+      if (!dataset || dataset.trim() === "") {
+        dataset = defaultName;
+        console.warn(`empty dataset configuration option - setting to '${dataset}'`);
+      } else {
+        dataset = dataset.trim();
+      }
+
+      this.defaultDataset = dataset;
+    } else {
+      if (opts.serviceName !== opts.serviceName.trim()) {
+        console.warn(`service name contains whitespace '${opts.serviceName}'`);
+      }
+      // if servicename is empty (whitespace) or starts with "unknown_service", use "unknown_service"
+      // or use trimmed service name
+      this.defaultDataset = opts.serviceName.startsWith("unknown_service") || opts.serviceName.trim() === ""
+        ? "unknown_service"
+        : opts.serviceName.trim();
+      if (opts.dataset) {
+        console.warn(`dataset should be empty - sending data to '${this.defaultDataset}'`);
+      }
+    }
 
     const libhoneyOpts = getFilteredOptions(opts);
 
@@ -105,6 +137,7 @@ module.exports = class LibhoneyEventAPI {
     this.honey.add({
       [schema.HOSTNAME]: os.hostname(),
       [schema.TRACE_SERVICE_NAME]: opts.serviceName,
+      [schema.TRACE_SERVICE_DOT_NAME]: opts.serviceName,
     });
   }
 
diff --git a/lib/propagation/honeycomb.js b/lib/propagation/honeycomb.js
index 23975ee..2949038 100644
--- a/lib/propagation/honeycomb.js
+++ b/lib/propagation/honeycomb.js
@@ -9,6 +9,13 @@ exports.TRACE_HTTP_HEADER = TRACE_HTTP_HEADER;
 const VERSION = "1";
 exports.VERSION = VERSION;
 
+let propagateDataset = true;
+
+exports.setPropagateDataset = setPropagateDataset;
+function setPropagateDataset(enabled) {
+  propagateDataset = enabled;
+}
+
 // assumes a header of the form:
 
 // VERSION;PAYLOAD
@@ -38,7 +45,7 @@ function marshalTraceContextv1(context) {
   let dataset = context.dataset;
 
   let datasetClause = "";
-  if (dataset) {
+  if (propagateDataset && dataset) {
     datasetClause = `dataset=${encodeURIComponent(dataset)},`;
   }
 
@@ -84,7 +91,9 @@ function unmarshalTraceContextv1(payload) {
         parentSpanId = v;
         break;
       case "dataset":
-        dataset = decodeURIComponent(v);
+        if (propagateDataset) {  
+          dataset = decodeURIComponent(v);
+        }
         break;
       case "context":
         contextb64 = v;
diff --git a/lib/schema.js b/lib/schema.js
index 4849c15..a634ec0 100644
--- a/lib/schema.js
+++ b/lib/schema.js
@@ -14,6 +14,7 @@ exports.TRACE_ID_SOURCE = "trace.trace_id_source";
 exports.TRACE_PARENT_ID = "trace.parent_id";
 exports.TRACE_SPAN_ID = "trace.span_id";
 exports.TRACE_SERVICE_NAME = "service_name";
+exports.TRACE_SERVICE_DOT_NAME = "service.name";
 exports.TRACE_SPAN_NAME = "name";
 
 // custom context fields will have this prefix
diff --git a/lib/util.js b/lib/util.js
index 603d723..fbe47ac 100644
--- a/lib/util.js
+++ b/lib/util.js
@@ -17,3 +17,9 @@ function captureStackTrace(skipFrames = 0, limitFrames = 10) {
   // the +1 here to get rid of the `Error\n` line at the top of the stacktrace.
   return frames.slice(1 + skipFrames).join("\n");
 }
+
+exports.isClassic = isClassic;
+
+function isClassic(writeKey) {
+  return !writeKey || writeKey.length == 32;
+}
\ No newline at end of file


Test patch:
diff --git a/lib/api/index.test.js b/lib/api/index.test.js
index 1c46d0f..ffd0043 100644
--- a/lib/api/index.test.js
+++ b/lib/api/index.test.js
@@ -10,17 +10,103 @@ jest.mock("../deterministic_sampler");
 
 beforeEach(() => {
   api._resetForTesting();
-  api.configure({ impl: "libhoney-event", transmission: "mock", writeKey: "abc123" });
+  api.configure({
+    impl: "libhoney-event",
+    transmission: "mock",
+    writeKey: "e38be416d0d68f9ed1e96432ac1a3380",
+  });
 });
 
-test("libhoney default config", () => {
+test("libhoney default config - classic", () => {
   const honey = api._apiForTesting().honey;
   expect(honey.transmission.constructorArg.apiHost).toBe("https://api.honeycomb.io");
   expect(honey.transmission.constructorArg.dataset).toBe("nodejs");
-  expect(honey.transmission.constructorArg.writeKey).toBe("abc123");
+  expect(honey.transmission.constructorArg.writeKey).toBe("e38be416d0d68f9ed1e96432ac1a3380");
+  expect(honey.transmission.constructorArg.userAgentAddition).toBe(
+    `honeycomb-beeline/${pkg.version}`
+  );
+  expect(honey._builder._fields["service_name"]).toBe("unknown_service:nodejs");
+  expect(honey._builder._fields["service.name"]).toBe("unknown_service:nodejs");
+});
+
+test("libhoney default config - non-classic", () => {
+  api._resetForTesting();
+  api.configure({
+    impl: "libhoney-event",
+    transmission: "mock",
+    writeKey: "d68f9ed1e96432ac1a3380",
+  });
+
+  const honey = api._apiForTesting().honey;
+  expect(honey.transmission.constructorArg.apiHost).toBe("https://api.honeycomb.io");
+  expect(honey.transmission.constructorArg.dataset).toBe("unknown_service");
+  expect(honey.transmission.constructorArg.writeKey).toBe("d68f9ed1e96432ac1a3380");
+  expect(honey.transmission.constructorArg.userAgentAddition).toBe(
+    `honeycomb-beeline/${pkg.version}`
+  );
+  expect(honey._builder._fields["service_name"]).toBe("unknown_service:nodejs");
+  expect(honey._builder._fields["service.name"]).toBe("unknown_service:nodejs");
+});
+
+test("libhoney config - non-classic - empty service name", () => {
+  api._resetForTesting();
+  api.configure({
+    impl: "libhoney-event",
+    transmission: "mock",
+    writeKey: "d68f9ed1e96432ac1a3380",
+    serviceName: "",
+  });
+
+  const honey = api._apiForTesting().honey;
+  expect(honey.transmission.constructorArg.apiHost).toBe("https://api.honeycomb.io");
+  expect(honey.transmission.constructorArg.dataset).toBe("unknown_service");
+  expect(honey.transmission.constructorArg.writeKey).toBe("d68f9ed1e96432ac1a3380");
+  expect(honey.transmission.constructorArg.userAgentAddition).toBe(
+    `honeycomb-beeline/${pkg.version}`
+  );
+  expect(honey._builder._fields["service_name"]).toBe("unknown_service:nodejs");
+  expect(honey._builder._fields["service.name"]).toBe("unknown_service:nodejs");
+});
+
+test("libhoney config - non-classic - whitespace service name", () => {
+  api._resetForTesting();
+  api.configure({
+    impl: "libhoney-event",
+    transmission: "mock",
+    writeKey: "d68f9ed1e96432ac1a3380",
+    serviceName: " "
+  });
+
+  const honey = api._apiForTesting().honey;
+  expect(honey.transmission.constructorArg.apiHost).toBe("https://api.honeycomb.io");
+  expect(honey.transmission.constructorArg.dataset).toBe("unknown_service");
+  expect(honey.transmission.constructorArg.writeKey).toBe("d68f9ed1e96432ac1a3380");
+  expect(honey.transmission.constructorArg.userAgentAddition).toBe(
+    `honeycomb-beeline/${pkg.version}`
+  );
+  console.log(honey._builder._fields["service_name"]);
+  expect(honey._builder._fields["service_name"]).toBe(" ");
+  expect(honey._builder._fields["service.name"]).toBe(" ");
+});
+
+test("libhoney config - non-classic - custom service name", () => {
+  api._resetForTesting();
+  api.configure({
+    impl: "libhoney-event",
+    transmission: "mock",
+    writeKey: "d68f9ed1e96432ac1a3380",
+    serviceName: " my-service ",
+  });
+
+  const honey = api._apiForTesting().honey;
+  expect(honey.transmission.constructorArg.apiHost).toBe("https://api.honeycomb.io");
+  expect(honey.transmission.constructorArg.dataset).toBe("my-service");
+  expect(honey.transmission.constructorArg.writeKey).toBe("d68f9ed1e96432ac1a3380");
   expect(honey.transmission.constructorArg.userAgentAddition).toBe(
     `honeycomb-beeline/${pkg.version}`
   );
+  expect(honey._builder._fields["service_name"]).toBe(" my-service ");
+  expect(honey._builder._fields["service.name"]).toBe(" my-service ");
 });
 
 test("startTrace starts tracking and creates an initial event, finishTrace sends it", () => {
diff --git a/lib/propagation/honeycomb.test.js b/lib/propagation/honeycomb.test.js
index 45b6f80..cd0f16c 100644
--- a/lib/propagation/honeycomb.test.js
+++ b/lib/propagation/honeycomb.test.js
@@ -80,7 +80,7 @@ cases(
 );
 
 describe("roundtrip", () => {
-  test("works", () => {
+  test("classic", () => {
     let contextStr = honeycomb.marshalTraceContext(testContext);
     expect(honeycomb.unmarshalTraceContext(contextStr)).toEqual({
       traceId: "abcdef123456",
@@ -93,4 +93,18 @@ describe("roundtrip", () => {
       },
     });
   });
+
+  test("non-classic", () => {
+    honeycomb.setPropagateDataset(false);
+    let contextStr = honeycomb.marshalTraceContext(testContext);
+    expect(honeycomb.unmarshalTraceContext(contextStr)).toEqual({
+      traceId: "abcdef123456",
+      parentSpanId: "0102030405",
+      customContext: {
+        userID: 1,
+        errorMsg: "failed to sign on",
+        toRetry: true,
+      },
+    });
+  });
 });