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

[telemetry] More user options #3056

Merged
merged 3 commits into from
Aug 19, 2017

Conversation

GabrielXia
Copy link
Contributor

Contains

Now the user can choose which field he wants to send in the metrics menu.

  • He can enable/disable a single field, e.g. the "language" field.
  • He can enable/disable a whole metric e.g. "System Context", then all fields in that metric will be enabled/disabled.
  • Click the check box "Send metrics to server" will enable/disable all fields.
  • If all fields in the metric is disabled, that metric will be disabled automatically. If one field is enabled, that metric will be enabled automatically.
    2017-08-09 10 40 01

Make all the string default to no_analysed in ELK, we can finally do some pie graphs :) The utility.terasology.org:5601 is updated. Here is an example proposed in #3024

2017-08-09 10 50 15

Outstanding before merging

For the headless server, we need to edit in terasology-server/config.cfg:

  • "telemetryEnabled": true to enable telemetry
  • "errorReportingEnabled": true to enable error reporting
  • copy bindingMap in /config.cfg to terasology-server/config.cfg to have more options :)

The telemetry in the headless server is too manual, if you have some more ideas, please let me know! Thanks :)

Thanks @rzats @oniatus @Cervator @skaldarnar @msteiger @qwc for review and comments!

@GooeyHub
Copy link
Member

GooeyHub commented Aug 9, 2017

Hooray Jenkins reported success with all tests good!

Copy link
Member

@Cervator Cervator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only spotted a couple tiny bits of code to comment about, but the game crashed with an NPE on testing it and returning to the main menu:

java.lang.NullPointerException: null
	at org.terasology.telemetry.TelemetryUtils.trackMetric(TelemetryUtils.java:67)
	at org.terasology.telemetry.TelemetrySystem.shutdown(TelemetrySystem.java:154)
	at org.terasology.engine.ComponentSystemManager.shutdown(ComponentSystemManager.java:188)

Seems to be from not resetting config.cfg - after deleting it and retrying everything was fine. Maybe you can harden the code to gracefully handle that situation?

Also a side suggestion: Have a button/checkbox for enabling or disabling all :-)

Finally: I had a somewhat random thought recently, do we have any sort of rate limiting for submitting data to the server? Not likely to be an issue for some time but could a user maliciously flood the metrics collector and break the server?

import java.util.HashMap;
import java.util.Map;

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep an eye out for these silly little empty javadoc blocks. They remain here and there and sometimes get copy pasted around. Just delete them or better yet write some javadoc! ;-)

this.bindingMap = bindingMap;
}

private Map<String, Boolean> bindingMap = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be up at the top of the class (Checkstyle violation)

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@GabrielXia
Copy link
Contributor Author

Thanks @Cervator @rzats so much for the test!

@Cervator thanks for the bug that you find. The nlp happens the first time you enable the telemetry, not open the metrics screen in the game and return to the main menu or quit. In this pr, I simply do the nlp check to fix the nlp. Ideally I should add all the telemetry field at the beginning. Because I'll do several changes in the next pr, could I solve this issue totally in the next pr? I could also update all the code in this pr :) That will make this pr more complicated

I think at the bottom of the metrics menu, if you click send metrics to server, it'll enable/diable all the fields. I like also the idea of limiting metrics, do you think it should be done in the client side? e.g. the same user isn't able to send 100 metrics to the server? But that might cause a problem when some devs are debugging the telemetry code

@Cervator Cervator merged commit 1b54224 into MovingBlocks:develop Aug 19, 2017
Cervator added a commit that referenced this pull request Aug 19, 2017
@Cervator
Copy link
Member

Retested - seems good, thanks :-) Merged! Always good to harden further but yeah that can be whenever.

Enabling all on actually selecting the metrics seems handy.

On throttling - it would have to be a server side setting and I wouldn't be surprised if logstash / snowplow / something already has related options. It would be something like throttling individual sources that are sending unrealistically high amounts of data (likely malicious or a bug)

@Cervator Cervator added Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience labels Aug 19, 2017
@Cervator Cervator added this to the Alpha 8 milestone Aug 19, 2017
@GabrielXia GabrielXia mentioned this pull request Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants