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

add throttle_state to tracking #2878

Merged
merged 3 commits into from Nov 8, 2018

Conversation

3 participants
@tedder
Contributor

tedder commented Nov 6, 2018

  • weave the throttle_state through pi_support and tracking. This should come in handy to see how many throttled Pis are out there, as well as if it is correllated with canceled prints.
  • upgraded disclosure of tracking information so it shows as INFO-level in the logs and all the information is dumped. If tracking.octoprint.org gets it, I should have it too :)
add throttle_state to tracking
- weave the throttle_state through pi_support and tracking. This should come in handy to see how many throttled Pis are out there, as well as if it is correllated with canceled prints.
- upgraded disclosure of tracking information so it shows as INFO-level in the logs and all the information is dumped. If tracking.octoprint.org gets it, I should have it too :)
@GitIssueBot

This comment has been minimized.

Collaborator

GitIssueBot commented Nov 6, 2018

Hi @tedder,

Thank you for your contribution! Sadly it looks like there is something wrong with this PR from your branch extended_pi_support_tracking to OctoPrint:staging/maintenance:

  • Your PR's target branch staging/maintenance is not among the whitelisted target branches: devel, maintenance. Please always create PRs against the devel branch for new big features, or the maintenance branch for bug fixes, improvements and new small features.

Please take a look at the section on PRs in the Contribution Guidelines and make sure that your PR follows them. Thank you!

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being, so don't expect any replies from me :) Your PR is read by humans too, I'm just not one of them.

@tedder

This comment has been minimized.

Contributor

tedder commented Nov 6, 2018

I thought staging/maintenance was right to get it into the RC, 'bot.

@foosel

This comment has been minimized.

Owner

foosel commented Nov 7, 2018

It is, but the bot doesn't know that ;) Ignore it...


First of all: I like the idea of adding info on the throttling state, great thinking! 👍

There's still an issue though here since the current setup doesn't support nested data. This is how stuff arrives on the server:

[07/Nov/2018:06:26:16 +0100] "GET /track/REDACTED/print_done/?origin=local&throttle_state=raw_value&throttle_state=current_overheat&throttle_state=past_issue&throttle_state=past_overheat&throttle_state=current_issue&throttle_state=past_undervoltage&throttle_state=current_undervoltage&file=2f8bf9808c12c571457b73f35eca8baaad36b07c&elapsed=2486 HTTP/1.1" 204 0 "-" "OctoPrint/1.3.10rc1"

So that structure needs to be flattened.

I'm also thinking it might be better to have the pi_support generate a custom event on status change and subscribe to that in the tracking plugin, and when the event is received send over the status update. That way it's easier to filter for, and the noise on the line is also reduced (our GET requests are limited in length as well by the HTTP spec).

Now, about the logging, I get where you are coming from, and I'm not against this, but it was done the way it was done for a simple reason: I don't want to see the instance UUIDs from people when they share their logs with me since that would de-anonymize them, and by logging this at INFO level I would ;)

How about modifying this to not include the URL after all, at least not on INFO level? I really don't want to get knowledge of any UUIDs if it can be avoided :)

Leaving this at "needs work" due to the two points above.

PR feedback: throttle state and uuid
- add a way to get the raw value only from throttle state (glad I already plumbed it into the object!)
- add warning about not sending uuid, modify the log entry too
@tedder

This comment has been minimized.

Contributor

tedder commented Nov 8, 2018

I know I can ignore the bot, but sometimes it's nice to talk back a little. :)

FWIW GET is probably limited to about 2k characters. Certainly the nested dict isn't helping. I reduced it to only send the raw_value for the throttle state. I thought about two values so we could do basic bitmath before sending (so history would be 0-5, current would be 0-5), which would keep the server from knowing how to calculate it.

I know what you mean about sending it as an event, but it's sitting on about 20 characters now, which to me is worthwhile to not have to correlate GET requests to figure out if a failed print previously triggered overvoltage. Thoughts?

How about modifying this to not include the URL after all, at least not on INFO level? I really don't want to get knowledge of any UUIDs if it can be avoided :)

As you can see I removed the URL and added a note to explain it. I think that's a great way to go.

Pushing the code and testing locally over the next 24h, but wanted to put it up for discussion.

@@ -219,6 +231,11 @@ def _track_printjob_event(self, event, payload):
track_event = "print_failed"
elif event == Events.PRINT_CANCELLED:
track_event = "print_cancelled"
else:
logger.info("unknown event, skipping: {}".format(event))

This comment has been minimized.

@foosel

foosel Nov 8, 2018

Owner

That should be debug, otherwise we'll spam the log.

@foosel foosel merged commit 98c84b6 into foosel:staging/maintenance Nov 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

foosel added a commit that referenced this pull request Nov 8, 2018

tracking: add throttle events, slightly change throttle payload
Also adjusted helper signature in pi_support plugin slightly.

Addon to #2878
@foosel

This comment has been minimized.

Owner

foosel commented Nov 8, 2018

I know what you mean about sending it as an event, but it's sitting on about 20 characters now, which to me is worthwhile to not have to correlate GET requests to figure out if a failed print previously triggered overvoltage. Thoughts?

Why not both :D I added the events, so it's now possible to figure out how many throttled instances are out there without them needing to restart or print - for that I also added a new toggle so no one can complain there. And I also left the throttle state on the print events. Tested it against the backend and things are looking great.

@tedder

This comment has been minimized.

Contributor

tedder commented Nov 8, 2018

Looks great. Thanks for the modifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment