Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support classic ingest keys, upgrade to libhoney 4.2.0 #726

Merged

Conversation

cewkrupa
Copy link
Contributor

@cewkrupa cewkrupa commented Mar 1, 2024

Which problem is this PR solving?

We recently updated libhoney-js to support classic ingest keys. This PR updates the beeline to this version of libhoney-js and swaps isClassic to use the new exported method from libhoney.

Short description of the changes

  • bumps libhoney-js version from 4.0.0 to 4.2.0
  • removes utils.isClassic in favor of libhoney.isClassic
  • Adds two tests that use classic/non-classic ingest keys to verify it works

@cewkrupa cewkrupa requested a review from a team as a code owner March 1, 2024 16:05
@cewkrupa cewkrupa requested a review from a team March 1, 2024 16:07
@cewkrupa cewkrupa added dependencies Pull requests that update a dependency file version: bump minor A PR that adds behavior, but is backwards-compatible. labels Mar 1, 2024
Copy link

@jharley jharley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Left one query about not duplicating libhoney reqires, but otherwise good 👍🏻

@@ -5,9 +5,9 @@ const process = require("process"),
tracker = require("../async_tracker"),
pkg = require("../../package.json"),
debug = require("debug")(`${pkg.name}:event`),
libhoney = require("libhoney"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need two require's for libhoney and LibhoneyImpl?

PS I prefer libhoney over LibhoneyImpl if we can just keep line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think we do need both, since the LibhoneyImpl is coming from ./libhoney.js in this repo while libhoney is coming direct from the libhoney-js package as libhoney.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I didn't notice the local reference for LibhoneyImpl 👍🏻 It's kinda weird we have a file named the same as one of our dependencies (which we maintain :)).

@MikeGoldsmith MikeGoldsmith added type: enhancement New feature or request and removed dependencies Pull requests that update a dependency file labels Mar 4, 2024
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks @cewkrupa

@@ -5,9 +5,9 @@ const process = require("process"),
tracker = require("../async_tracker"),
pkg = require("../../package.json"),
debug = require("debug")(`${pkg.name}:event`),
libhoney = require("libhoney"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I didn't notice the local reference for LibhoneyImpl 👍🏻 It's kinda weird we have a file named the same as one of our dependencies (which we maintain :)).

@MikeGoldsmith MikeGoldsmith merged commit 81ea026 into main Mar 4, 2024
3 checks passed
@MikeGoldsmith MikeGoldsmith deleted the cewkrupa/support-classic-ingest-keys-bump-libhoney branch March 4, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants