api3dao / airnode

Airnode monorepo
https://docs.api3.org/
MIT License
165 stars 72 forks source link

Pending logs are printed with incorrect timestamp #1570

Open aTeoke opened 1 year ago

aTeoke commented 1 year ago

Airnode uses a pattern of “pending” logs, which makes functions store the logs and return them to be logged by some higher level function.

This is problematic since, the timestamp gets assigned once the logs are printed https://github.com/api3dao/airnode/blob/minor-examples-improvement/packages/airnode-node/src/logger.ts#L61 .

There are two options:

  1. This pattern should be revisited whether there is not a better solution
  2. Just add a timestamp to the pending logs and not just by the higher level function
dcroote commented 1 year ago

The above link is broken as the branch was deleted- here is a permalink from a commit on that branch: https://github.com/api3dao/airnode/blob/ee2a5a53708aca2416403f081799611c3ee6ec2a/packages/airnode-node/src/logger.ts#L61

andreogle commented 1 year ago

The reason for the "pending logs" was that I was trying to avoid any and all global variables, as they may have resulted in unexpected/unintended behaviour (e.g. stale cached data). I went with the approach of trying to keep ALL variables within the scope of the specific coordinator cycle so that they would be guaranteed to be fresh. I wasn't familiar with the idiosyncrasies of the different Serverless environments and this felt like the safest approach.

Specifically, I was worried about the logging metadata (coordinator ID, provider name, chain etc.). I also didn't want to pass these additional variables to all of the lower level functions for "cleaner"/simpler functions and easier testing. The functions that log something return a tuple and ESLint makes sure that there are no unused variables. i.e. the first tuple element (the logs) must be used in some way.

I'm aware that this approach has tradeoffs and the reasons are certainly not obvious to new readers of the code. I'm happy for this to be reconsidered, but just wanted to make sure the initial reasons were understood.

Siegrift commented 1 year ago

Thanks @andreogle , I think the implementation is justified and works well for servlerless. Since we plan to refactor Airnode to single lambda in the future we can revert to a more simple approach. Logging is hard and there is some inherent complexity to it:

Apart from the pending logs timestamp bug, I see other disadvantages of that concept. It's a good feature that we can accumulate the logs from "slaves" and print them by some "master" function, but this is error-prone. The logs need to be merged during accumulation (which is harder to read), and the master needs to remember to print them out (otherwise they are lost). These logs are propagated among many functions and it's hard to reason about this... I think we should just simply log when we need/want to log and let the logger tool process the log. I strongly think we should have structured logs so that we can easily filter/search stuff... (Not sure if we need a special tool for this or the native cloud provider log services are enough)...