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

made loki connection optional #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evansteelepdx
Copy link
Contributor

@evansteelepdx evansteelepdx commented Apr 14, 2022

Starting from scratch resulted in several errors due to missing values from the loki object in the default settings.json file

  • added base cases for Loki query methods
  • added check for base_url content in settings.json parsing to avoid errors
  • added Loki config object to default settings.json file creation

@evansteelepdx
Copy link
Contributor Author

resolves #64

@mdiller
Copy link
Owner

mdiller commented Apr 19, 2022

Thanks for the pull request! So most of the changes you made were around moving loki to be a thing included by default in config files but I think I'd prefer to go with the approach of not having the "loki" object added to the config file at all in cases where people arent using it.

Given that, I think that theres actually only one line that breaks the current mangobyte which is this one: LOKI_APPLICATION_NAME = settings.loki["application"]. We could fix this in a couple ways, either just adding an if to it so it only runs that line if settings.loki exists, or just have that constant be a part of the class when it gets initialized, or even just not create a variable for it and put it directly in the strings that use it.

@evansteelepdx
Copy link
Contributor Author

evansteelepdx commented Apr 19, 2022

@mdiller I agree that there are probably too many checks in this case, and that we can eliminate all unhandled exceptions by checking on that line. However, since each bot action logs a request to the Loki endpoint, you'll still get a stacktrace from the Loki emitter that'll fill up your local logs quickly

requests.exceptions.MissingSchema: Invalid URL '/loki/api/v1/push': No scheme supplied. Perhaps you meant http:///loki/api/v1/push?

  • Example where base_url is blank

If you're fine with lots of local handled exceptions, then it's probably fine to keep in. I've added a new commit with the only 2 required change points:

  • settings.json file read
  • looping request in update_botstats

- rolled back previous commit
- fewer checks for loki configuration keys
@mdiller
Copy link
Owner

mdiller commented Jun 21, 2022

Hey! Circling back to this issue and taking a fresh look at it, I think the only real problem here is the line in cogs/general.py that does LOKI_APPLICATION_NAME = settings.loki["application"]. That should only be getting called if loki exists, so if we just change that to:

if settings.loki:
  LOKI_APPLICATION_NAME = settings.loki["application"]

Then that should fix the problem. I think I'd like to keep the rest of the behavior the same, that way theres no problem if nobody adds a "loki" key in the settings.json, and then if they do add "loki" but they didnt do the rest of the args right, it should error at them, to let them know that theyre doing something wrong.

Sound good?

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