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

Add new package supporting Edge runtime (eg. Cloudflare Workers) #78

Merged
merged 13 commits into from
Sep 13, 2023

Conversation

PetrHeinz
Copy link
Member

New package supporting running in edge runtime, such as Cloudflare Workers.

Resolves #62

@PetrHeinz PetrHeinz changed the title Copy node package to edge (only naming changes) with a separate Jest project config Add new package supporting Edge runtime (eg. Clouflare Workers) Sep 8, 2023
@PetrHeinz PetrHeinz changed the title Add new package supporting Edge runtime (eg. Clouflare Workers) L-102 Add new package supporting Edge runtime (eg. Clouflare Workers) Sep 8, 2023
@PetrHeinz PetrHeinz changed the title L-102 Add new package supporting Edge runtime (eg. Clouflare Workers) Add new package supporting Edge runtime (eg. Clouflare Workers) Sep 8, 2023
@PetrHeinz PetrHeinz changed the title Add new package supporting Edge runtime (eg. Clouflare Workers) Add new package supporting Edge runtime (eg. Cloudflare Workers) Sep 11, 2023
@PetrHeinz PetrHeinz requested a review from Tatarkow September 12, 2023 13:42
@PetrHeinz PetrHeinz marked this pull request as ready for review September 12, 2023 13:42
@PetrHeinz
Copy link
Member Author

PetrHeinz commented Sep 12, 2023

Example usage in Cloudflare Worker:

import { Logtail } from "@logtail/edge"

const baseLogger = new Logtail("<BETTER_STACK_SOURCE_TOKEN>")

export default {
  async fetch(request, env, ctx) {
    const logger = baseLogger.withExecutionContext(ctx);

    logger.info("Logging with structured data.", { field: "context value", myArray: [1, 2, 3] });
    logger.warn("Warning: This function is just Hello World.");

    return new Response('Hello world!');
  },
};

image

@PetrHeinz PetrHeinz requested review from curusarn and removed request for Tatarkow September 12, 2023 14:36
Comment on lines 83 to 86
this._warnedAboutMissingCtx = true;
console.warn(
"ExecutionContext hasn't been set via `withExecutionContext` method. Logs may not reach Better Stack unless you manually call `ctx.waitUntil(log)`.",
);
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be quite dangerous - if there's no ctx.waitUntil, await logger.log or await logger.flush it would mean the logs will probably not get sent to Better Stack. The warning is also quite hard to notice (depending on how you run it).

Maybe this should throw an exception instead to make sure it's correctly set up...

@aroman, @curusarn what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why people might not want to use the withExecutionContext? E.g. maybe there could be workers that doesn't make the context easily available?

I would consider throwing an exception if there is never a good reason to not use withExecutionContext.
But it's still a little weird for logging library to bring down people's worker code.

How about send the warning to both console and Better Stack? Plus flush after the warning to make sure people at least see the warning in Live tail. I imagine seeing the warning instead of actual logs in Live tail could work well.

Also could you make sure that the warning gives people concrete instructions how to fix the issue? 🙏
E.g. now I feel like the warning tells me that the easiest solution is to add ctx.waitUntil(log) to my worker code. But the recommended way is to use withExecutionContext().
Could you change the warning to make it clearer how to fix the issue? Link to docs page might work well. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the warning + flush method, I'll just have to test it...

I don't see a good reason not use withExecutionContext, but then again, I might be mistaken. Maybe in some future version we'll make a feature toggle for disabling the warning.

And I'll reword it to make sure it is understandable 👍 Thanks for the feedback.

{
"name": "@logtail/edge",
"version": "0.4.6",
"description": "Logtail.com - Edge runtime logger",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Logtail.com - Edge runtime logger",
"description": "Better Stack Edge runtime logger (formerly Logtail)",

Let's please use Better Stack instead of Logtail. 🙏

Could you also add "Better Stack" and "Better Stack Logs" to keywords (keep Logtail)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll unify that in a new PR, it's still Logtail everywhere, probably

"serverless",
"Edge"
],
"author": "Logtail <hello@logtail.com>",
Copy link
Contributor

Choose a reason for hiding this comment

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

Better Stack <hello@betterstack.com> 🙏

Copy link
Contributor

@curusarn curusarn left a comment

Choose a reason for hiding this comment

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

Left some comments regarding package description and the "edge warning behavior". Looks good overall! Thanks.

@PetrHeinz PetrHeinz merged commit b522a57 into master Sep 13, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for edge environments (Cloudflare Workers, Vercel Serverless, etc)
2 participants