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

fix(serverless): Axiom logger #216

Merged
merged 6 commits into from
Oct 29, 2022
Merged

fix(serverless): Axiom logger #216

merged 6 commits into from
Oct 29, 2022

Conversation

bahlo
Copy link
Contributor

@bahlo bahlo commented Oct 26, 2022

Hey everyone, this project looks exciting! I've noticed you're using Axiom so I fixed the logger for you, hope that's alright 😊

About

This PR fixes a bug where only the first log event would've been sent to Axiom.
Instead of grabbing the first value from the receiver, it turns the receiver into an async stream and passes it to ingest_stream for ingestion.
There's more things that can be improved (e.g. ingest_stream should log errors and restart instead of panicking on error) but this is meant to be a first step.

It also changes the way Axiom is detected to using the client constructor which will automatically configure from environment variables.

And it renames the timestamp field name to _time so it's properly detected by Axiom.

Please let me know if you have any questions regarding Axiom, adding support for the log crate is already on our list 😁

bahlo added 2 commits October 26, 2022 10:47
Instead of manually checking environment variables we can instantiate
the Axiom client and it will inherit configuration from the environment.
This fixes a bug where only the very first event would've been sent to
Axiom.
The receiver is now transformed into an async stream and passed to
`ingest_stream`, which will automatically send events every second or in
batches of 1000 (whichever happens first).

It also renames the `timestamp` field to `_time` so Axiom recognizes it.

And it upgrades axiom-rs to 0.6 because that release added the flush
every second functionality.
Copy link
Member

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and improvements, smart use of ingest_stream! Love Axiom, really great product.

There's more things that can be improved (e.g. ingest_stream should log errors and restart instead of panicking on error) but this is meant to be a first step.

Definitely agree on this point, I'll log an issue for this.

Could you also add a changeset describing your changes by running pnpm changeset (and selecting the serverless package) at the root of the project?

bahlo added 2 commits October 27, 2022 09:03
Using `log::warn` does nothing if the logger is not setup.
@bahlo bahlo requested a review from QuiiBz October 27, 2022 07:07
@bahlo
Copy link
Contributor Author

bahlo commented Oct 27, 2022

@QuiiBz Thanks for the kind words — I've added the changeset 💯

We should also probably drop the Sender on exit so the Receiver/Stream is closed and remaining events are sent 🤔

@QuiiBz
Copy link
Member

QuiiBz commented Oct 27, 2022

We should also probably drop the Sender on exit so the Receiver/Stream is closed and remaining events are sent 🤔

Agree, could you add an impl Drop for SimpleLogger? We'll be ready to merge after this!

@bahlo
Copy link
Contributor Author

bahlo commented Oct 28, 2022

Agree, could you add an impl Drop for SimpleLogger? We'll be ready to merge after this!

I don't think it's that would fix it, the SimpleLogger instance is passed to set_boxed_logger, which Box::leaks it. Also the Sender should be dropped automatically if the parent struct SimpleLogger is dropped. Or am I missing something?

My proposal would be to have a kind of guard that drops the Sender (which should be wrapped in an option). And while we're on that, we should probably return the promise of the spawned task as well and join! on that to make sure logs are flushed.

Does that make sense?

@QuiiBz
Copy link
Member

QuiiBz commented Oct 28, 2022

I don't think it's that would fix it, the SimpleLogger instance is passed to set_boxed_logger, which Box::leaks it. Also the Sender should be dropped automatically if the parent struct SimpleLogger is dropped. Or am I missing something?

Yeah, you're right, I've misunderstood your point. I think we could also use flush from Log's trait when the process exits.

This is a std::sync::RwLock which you generally shouldn't use in an
asyncc environment but we can't await in the `log::Log` trait sooo I
hope this is fine?
The RwLock should never block while the application is running.
@bahlo
Copy link
Contributor Author

bahlo commented Oct 28, 2022

@QuiiBz I pushed 66cb4b4 but I'm not sure this is the best solution. Happy about any feedback 👍

Copy link
Member

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! I only have a small suggestion:

@bahlo bahlo requested a review from QuiiBz October 29, 2022 09:04
@QuiiBz QuiiBz enabled auto-merge (squash) October 29, 2022 09:09
@QuiiBz QuiiBz changed the title Fix Axiom logger fix(serverless): Axiom logger Oct 29, 2022
Copy link
Member

@QuiiBz QuiiBz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot

@QuiiBz QuiiBz merged commit b5d47cb into lagonapp:main Oct 29, 2022
@QuiiBz QuiiBz mentioned this pull request Oct 29, 2022
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.

2 participants