Update Telemetry format and allow to customize it#195
Conversation
lbalmaceda
force-pushed
the
update-telemetry
branch
from
42c55b6 to
f54e185
Compare
| public Telemetry(String name, String version, String core) { | ||
| this.name = name; | ||
| this.version = version; | ||
| this.core = core; |
There was a problem hiding this comment.
What is core here? If it's used to extend this SDK's telemetry, let's put the SDK name instead to be more clear.
There was a problem hiding this comment.
core for this library is nothing (null). But those libs that use this internally, would expose as core the version of auth0-java (this sdk). In other SDKs we named it "libraryVersion", I can change to that if you agree. Remember though env includes core as per the RFC.
There was a problem hiding this comment.
RFC mentioned core long ago (mostly in the context of libs using auth0.js) but it's not there now. Let's use the library name itself, that will be much clearer when pulling reports. I'l add that to the RFC now.
| public String getValue() { | ||
| Map<String, String> values = new HashMap<>(); | ||
| Map<String, Object> values = new HashMap<>(); | ||
| if (name != null) { |
There was a problem hiding this comment.
Would we ever send telemetry if the name was null? Or the version?
There was a problem hiding this comment.
The only scenario this would happen is if the user overrides it and sets an explicit null for them. I don't think we should send the telemetry in that case. But then we should change that on the android side as well (PR had the same diff and you approved it) auth0/Auth0.Android#209. Side note, env will always have at least the java version regardless. But again, without name and version it wouldn't make sense. Your call!
There was a problem hiding this comment.
I don't know what the behavior in the reports would be with a blank name or version. Name is the most critical, without that we have nothing to go off of so I would skip sending in that case. For version ... that would likely just be a mistake and if it was a non-Auth0 developer extending for some reason, they wouldn't see the problem. So ... name required, I don't think version needs to be. Thoughts?
Sorry if I missed that in the other library. If there are questions around how the telemetry should behave, can you call that out with a comment next time so it's not missed?
| value = tmpValue; | ||
| } | ||
|
|
||
| public String getName() { |
There was a problem hiding this comment.
When are these getters used? Just for testing?
There was a problem hiding this comment.
Yep. I want to expose something so libraries overriding this can check everything is being replaced fine 👍
2 participants
Changes
AuthAPIandManagementAPIclients to set a new custom telemetry instance.References
SDK-700Testing
Checklist