-
Notifications
You must be signed in to change notification settings - Fork 306
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
Enable telemetry #6415
Enable telemetry #6415
Conversation
bc756f1
to
ae61c7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a bit late for that but since its not a breaking change yet and only implies in small changes in here and in the lambda code its worth considering. @adriansmares @KrishnaIyer
For now I would keep the structure as is. We can always change it later on and handle different formats in the collector.
ae61c7c
to
e65c5b7
Compare
e65c5b7
to
44951fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please fix these small nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing more to add.
44951fe
to
25f52a5
Compare
Writing it here the steps to test the telemetry feature on the CLI and in the Stack. Stack:
telemetry:
enable: true
entity-count-telemetry:
enable: true
interval: 10s
num-consumers: 1
target: "http://localhost:8080" Obs: Don't have anything running on When starting the Stack you should see:
And after a few seconds you should see the following:
Then after 10 seconds you should see the same message again.
telemetry:
enable: true
entity-count-telemetry:
enable: true
interval: 10s
num-consumers: 1
target: "https://telemetry.thethingsstack.io/collect" You should see the following logs:
The contents of this message have to match the values present in the database, meaning that if you created 10 applications the values of
telemetry:
enable: false
entity-count-telemetry:
enable: true
interval: 10s
num-consumers: 1
target: "http://localhost:8080" This by itself should stop all the telemetry procedures, so when running the Stack you should not see any of the messages listed on the first test item, such as:
telemetry:
enable: true
entity-count-telemetry:
enable: false
interval: 10s
num-consumers: 1
target: "http://localhost:8080" With this configuration you should still see the logs informing that the telemetry is enabled but there should not be any message regarding the
CLIThe CLI doesn't have a configurable interval for sending telemetry data. Before each test case, you will have to delete the file used to store when was the last time a telemetry message was sent. The file itself is stored in the Operating system's cache folder, where that might be on your system is detailed below:
Most likely if you are on Linux it will be on In addition to that, in all the following test cases the CLI's command used for testing is non specific, meaning that it should work if all the commands. I suggest trying to test it with different commands in order to ensure that this is valid, a few example are:
telemetry:
enable: true
target: "http://localhost:8080" There should be a failure message in the DEBUG log, like the following:
telemetry:
enable: false
target: "http://localhost:8080" Enable DEBUG logs in order to validate that an error log is not shown (meaning that no telemetry message POST is performed). You should not see any of the logs mentioned in the previous test items. |
Summary
References #6081
Enables telemetry for the Stack and the CLI, this sets the default value to
true
and it defines the value of thetarget
at which the telemetry information is sent.The main points of this PR are:
changelog
messageChanges
uuid
for telemetry/cli. Has the same effect since both use the RFC 4122 but Google's package should be one used on my opinion.INFO
messages for when telemetry is enable on CLI and/or IS.Testing
Regressions
As this is a new feature, there aren't any regressions that I can imagine but this is the last change to have breaking changes.
For example the current message structure works around the concept that each field on the main json body represents a data collected in general, it might be interesting to encapsulate a bit more since we plan to add telemetry in other places, for example:
Might be a bit late for that but since its not a breaking change yet and only implies in small changes in here and in the lambda code its worth considering. @adriansmares @KrishnaIyer
Notes for Reviewers
lorawan-stack/pkg/telemetry/exporter/config.go
Line 37 in 44951fe
This allows users to set their own values used to generate the hash, allowing them to disassociate their console URLs and still contribute telemetry data.
Checklist
README.md
for the chosen target branch.CHANGELOG.md
.CONTRIBUTING.md
, there are no fixup commits left.