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

Support Opt-In behaviour for logging requests #100

Open
bradykieffer opened this issue Jul 24, 2020 · 6 comments
Open

Support Opt-In behaviour for logging requests #100

bradykieffer opened this issue Jul 24, 2020 · 6 comments

Comments

@bradykieffer
Copy link

bradykieffer commented Jul 24, 2020

Requests logging opt-in

I have a number of services that have certain endpoints that we would love to use request logging for, but, they also contain a number of endpoints that are much more "spammy" and we don't really get a benefit from emitting logs for each request. What would your thoughts be on updating the framework to support a setting making request logging opt-in. Something like

REQUEST_LOGGING_DEFAULT_OPT_IN: bool = True  # By default opt in to all requests being logged

Then, a decorator could be used like opt_in_to_logging(silent=True), which could raise an error if REQUEST_LOGGING_DEFAULT_OPT_IN is true

Misc

  • It would also be really nice to be able to globally specify silent=False on no_logging - I wouldn't mind implementing this!
    • It might be nice to call out that by default setting no_logging will still generate a log message too
  • I really like packages like this for these types of operations, I was going to hand roll this code and the implementation here is just much nicer to use
    • Anything that I pitch here I'd 100% be down to contribute, I just want to avoid writing code for features that aren't actually wanted within the library
@tclancy
Copy link
Contributor

tclancy commented Jul 24, 2020

That definitely makes sense, we have a few endpoints we aren't particularly interested in or are only interested in if they error, so maybe instead of a hard true/ false the decorator could accept a condition or lambda just to make this even more fun for you to implement?

I can certainly see users who only want to use the package in certain cases but for backwards-compatibility the default would still need to be everything, yeah.

@bradykieffer
Copy link
Author

bradykieffer commented Jul 24, 2020

@tclancy what about instead of a true false in the setting we made it an enum / well formed string, something like:

REQUEST_LOGGING_DEFAULT_OPT_IN_LEVEL: str = "ALL" # Default value, opt in to logging all requests

Where it could also be something like ERRORS, NONE, etc. I don't suspect that will break anything.

EDIT: (as a side note, I'm not opposed to implementing a lambda, I'm just wondering what could be the easiest thing to use / what level of sophistication would be needed from the get go)

Also as a side note, I just realized that the silent flag hasn't yet been published, is that something that's going to happen soon by any chance?

@tclancy
Copy link
Contributor

tclancy commented Jul 24, 2020

That would work, it just wouldn't suit our use case.

I just realized that the silent flag hasn't yet been published, is that something that's going to happen soon by any chance?

Argh, I will try to do that as soon as I can; I've just switched to a new laptop so it won't be until next week, but if you don't see it by Friday, please ping me here.

@bradykieffer
Copy link
Author

May as well get it to suit your use case + mine so it's more immediately useful 😄

@tclancy
Copy link
Contributor

tclancy commented Jul 31, 2020

I haven't forgotten about this, just a crazy week.

@bradykieffer
Copy link
Author

@tclancy I also haven't forgotten, I just put up a PR for this: #103

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

No branches or pull requests

2 participants