Skip to content

lambda-extension: remove logging #384

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

Closed

Conversation

simonrw
Copy link

@simonrw simonrw commented Dec 24, 2021

Issue #, if available:

Description of changes:

When running on AWS lambda I encountered the following error:

thread 'main' panicked at 'Could not determine the UTC offset on this
system. Possible causes are that the time crate does not implement
"local_offset_at" on your system, or that you are running in a
multi-threaded environment and the time crate is returning "None" from
"local_offset_at" to avoid unsafe behaviour. See the time crate's
documentation for more information.
(https://time-rs.github.io/internal-api/time/index.html#feature-flags):
IndeterminateOffset',
/home/simon/.cargo/registry/src/gb.xjqchip.workers.dev-1ecc6299db9ec823/simple_logger-1.16.0/src/lib.rs:409:85

This means that the simple logger setup could not find the current time.
Instead, we can print to stdout which is also collected by CloudWatch.
We can leave more sophisticated control of the output to the user.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

When running on AWS lambda I encountered the following error:

```
thread 'main' panicked at 'Could not determine the UTC offset on this
system. Possible causes are that the time crate does not implement
"local_offset_at" on your system, or that you are running in a
multi-threaded environment and the time crate is returning "None" from
"local_offset_at" to avoid unsafe behaviour. See the time crate's
documentation for more information.
(https://time-rs.github.io/internal-api/time/index.html#feature-flags):
IndeterminateOffset',
/home/simon/.cargo/registry/src/gb.xjqchip.workers.dev-1ecc6299db9ec823/simple_logger-1.16.0/src/lib.rs:409:85
```

This means that the simple logger setup could not find the current time.
Instead, we can print to stdout which is also collected by CloudWatch.
We can leave more sophisticated control of the output to the user.
@simonrw
Copy link
Author

simonrw commented Dec 24, 2021

Happy to discuss further how to handle this issue properly if required. Perhaps it's user error on my part.

@bahildebrand
Copy link
Contributor

I'm a little confused what the issue is here. Can you give a little more detail on your setup? Are you running theses examples directly, or basing your code off of them?

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

That code serves as examples, you don't really need it to run extensions. Wouldn't it be better to fix the underlying problem than leaving it for everyone to figure out how to setup logging?

I believe the examples should work again if you add this to the dev-dependencies:

time = { version = "0.3.5", features = ["local-offset"] }

See https://time-rs.github.io/internal-api/time/sys/local_offset_at/index.html

@bahildebrand
Copy link
Contributor

Ah this makes more sense now. Nice catch @calavera

@davidbarsky
Copy link
Contributor

I mentioned this to @nmoutschen over Twitter DMs, but I think the right solution here is to not use local time in when logging and instead default to UTC and rely on the log ingestion system (CloudWatch or otherwise) to determine the local ingestion time and convert it to a some localized time instead. If a change is to be made, I'd probably get rid of simple_logger and replace it with env_logger or tracing_subscriber.

(An important disclosure: I help maintain some tracing libraries, including tracing_subscriber. However, I think defaulting to tracing_subscriber is a good choice because awslabs/aws-sdk-rust also uses tracing and I strongly suspect that the other big library people use with this crate is the AWS SDK.)

@calavera
Copy link
Contributor

(An important disclosure: I help maintain some tracing libraries, including tracing_subscriber. However, I think defaulting to tracing_subscriber is a good choice because awslabs/aws-sdk-rust also uses tracing and I strongly suspect that the other big library people use with this crate is the AWS SDK.)

yeah, that's a good point. The only reason why these examples use simple logger is because the other examples also use it, and I wanted to keep them similar. I normally use tracing for all my logs.

@simonrw
Copy link
Author

simonrw commented Dec 25, 2021

Sorry for the brief description of the issue, I realise that wasn't enough information at the time of posting. Thank you all for understanding my issue and resolving it.

I've tried using tracing_subscriber for setting up the logger, and this works well. I think since the examples already use tracing::info!, we could update the log setup to use tracing_subscriber. In my case, I have replaced the SimpleLogger with:

use tracing_subscriber::util::SubscriberInitExt;

#[tokio::main]
async fn main() -> Result<(), Error> {
    tracing_subscriber::fmt().json().finish().init();
    // ...
}

I agree with logging in UTC - I believe this should be the default. I'm happy to update this PR to use tracing_subscriber if we want?

@coltonweaver
Copy link
Contributor

Sorry for the brief description of the issue, I realise that wasn't enough information at the time of posting. Thank you all for understanding my issue and resolving it.

I've tried using tracing_subscriber for setting up the logger, and this works well. I think since the examples already use tracing::info!, we could update the log setup to use tracing_subscriber. In my case, I have replaced the SimpleLogger with:

use tracing_subscriber::util::SubscriberInitExt;

#[tokio::main]
async fn main() -> Result<(), Error> {
    tracing_subscriber::fmt().json().finish().init();
    // ...
}

I agree with logging in UTC - I believe this should be the default. I'm happy to update this PR to use tracing_subscriber if we want?

If you are willing to do it in this PR we're more than happy to review it! Otherwise I agree with @davidbarsky, and think we should at the very least open an issue for this separately and start working on it.

@calavera
Copy link
Contributor

calavera commented Jan 6, 2022

hey @mindriot101, thanks a lot for bringing this up. We've decided to replace all the logging examples with tracing, so this change is not necessary anymore, see #389. I'm going to close this PR 🙌

@calavera calavera closed this Jan 6, 2022
@simonrw
Copy link
Author

simonrw commented Jan 6, 2022

That's great, I'm glad this discussion happened at least 👍

@nmoutschen nmoutschen added the 0.5 label Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants