Skip to content
This repository has been archived by the owner on Feb 16, 2020. It is now read-only.

Send Log Messages Remotely #2378

Closed
wants to merge 1 commit into from
Closed

Send Log Messages Remotely #2378

wants to merge 1 commit into from

Conversation

crypto49er
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

  • What is the current behavior? (You can also link to an open issue here)

Warning and Error messages are only shown in terminal.

  • What is the new behavior (if this is a feature change)?

Warning and Error messages will be shown in Telegram and/or Pushbullet if user have them enabled.

  • Other information:

Most of the code came from this pull request (#2103). I compartmentalize the remote log function so the remote control via Telegram can be added on top in a later pull request.

@askmike
Copy link
Owner

askmike commented Jul 28, 2018

Hey! Great idea. Though unfortunately I am still afraid of the following:

Have you seen #1850? I think there is a lot of overlapping func.

Gekko has only one way of passing messaging between plugins, and that way is the event system, see here: https://gekko.wizb.it/docs/internals/events.html

This PR bypasses that system completely which I don't think is the best way forward. The event system is kind of a strange beast (it's something gekko specific), but (when used properly) it comes with a few benefits:

  • They are the only API that plugins have for receiving/sending data.
  • Gekko can turn them on/off based on the "mode" (backtest, live gekko, etc)
  • Gekko can give some guarantees about getting them in a specific order (probably not that relevant in this case), see here: https://forum.gekko.wizb.it/thread-56579.html
  • Events are automatically broadcasted to the UI as well (over the websocket API).
  • There are some other ways into getting insights into events (such as the event logger, etc)

I rather keep the "log" module for low level stuff, everything the user cares about (in case of no errors/crashes/etc) should flow through the events API.

Let me know your thoughts!

@crypto49er
Copy link
Contributor Author

Hi Mike,

I agree this PR bypasses the event system and might make things difficult to troubleshoot in the future. But what is the best way to have warning and error messages sent to plugins so the user is aware of them? You mentioned the event logger, so I probably will look into that some more. My guess is the messaging plugins (Telegram, Pushbullet, Email) have to be modified to listen to the event logger (or directly to events system)?

@askmike
Copy link
Owner

askmike commented Jul 29, 2018

My guess is the messaging plugins (Telegram, Pushbullet, Email) have to be modified to listen to the event logger (or directly to events system)?

It's quite easy to add a new event to Gekko, but now I am thinking about this some more I see it's not as trivial:

Right now events are messages, anyone (meaning: every plugin) can listen to them but only 1 plugin can emit them. That won't work here, as I think warnings and errors can come from a number of different plugins. (It's not hard to create a special event that can be broadcasted from anywhere, but I'd like to think about a clean API design).

Also note that some things you can consider an error already have their own event (which makes it easier listen to specifically those), for example:

I'm not saying that is the best way perse, but if we introduce a new concept maybe we should change these ones as well so that Gekko only has 1 way to do something?


As to your new feature: what kind of error messages and warnings do you want to send over telegram?

@crypto49er
Copy link
Contributor Author

I think tradeAborted, tradeCancelled and tradeErrored covers the errors and warnings I'm looking into for now. I will try to modify the Telegrambot plugin to see if I can get it to listen and report these errors.

@askmike
Copy link
Owner

askmike commented Jul 29, 2018 via email

@crypto49er
Copy link
Contributor Author

Thanks Mike! I just submitted a new PR that uses the event system to emit trades to Telegram.

@crypto49er crypto49er closed this Jul 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants