[android] added performance event #13795
Conversation
osana
added
Android
| @SerializedName("sessionId") | ||
| private final String sessionId; | ||
|
|
||
| @SerializedName("data") |
There was a problem hiding this comment.
FYI if the field name matches the serialized variant, there is no need to declare @SerializedName
| @SerializedName("data") | ||
| private final Bundle data; | ||
|
|
||
| private static final SimpleDateFormat dateFormat = |
There was a problem hiding this comment.
when using static final, make field name uppercase
| * Register performance event. | ||
| * @param data performance event data | ||
| */ | ||
| void onPerformanceEvent(Bundle data); |
There was a problem hiding this comment.
While we are introducing a performance event that can be reused for multiple purposes, I would prefer keeping the TelemetryDefinition interface to be as concrete as possible. Some users swap out our telemetry with their own and wouldn't know how to implement onPerformanceEvent(Bundle data). Having a method with tile latency and adding a long duration parameter would make it more clear and easier for those users to integrate.
| @Override | ||
| public void onPerformanceEvent(Bundle data) { | ||
| if (data != null && !data.isEmpty()) { | ||
| telemetry.push(new PerformanceEvent("What is session?", data)); |
There was a problem hiding this comment.
"What is session?" -> SessionIdentifier.java
| import java.util.Date; | ||
| import java.util.Locale; | ||
|
|
||
| public class PerformanceEvent extends Event { |
| @@ -0,0 +1,69 @@ | |||
| package com.mapbox.mapboxsdk.module.telemetry; | |||
There was a problem hiding this comment.
if you are adding this class to the public API of telemetry defintion, you should place it under module as these files are removed by some clients to roll their own.
Note however that in a another comment, I'm indicating to remove this class from the public API so no action should be taken, just an FYI.
There was a problem hiding this comment.
@tobrun I felt that PerformanceEvent constructor should not be public and either:
- I should add public PerformanceEventFactory that creates those events
- or keep it together with the impl classes. (i.e. in the module).
|
|
||
| /** | ||
| * Register performance event. | ||
| * @param data performance event data |
| Bundle data = new Bundle(); | ||
| data.putString("metrics_name", "metrics_latency"); | ||
| data.putLong("latency", response.receivedResponseAtMillis() - response.sentRequestAtMillis()); | ||
| telemetry.onPerformanceEvent(data); |
There was a problem hiding this comment.
instead of pushing out an event with each tile load, we could batch them?
osana
force-pushed
the
osana-telem
branch
4 times, most recently
from
99e9713 to
954a5ed
Compare
|
|
||
| private final String event; | ||
|
|
||
| private final String created; |
There was a problem hiding this comment.
| private final String created; | |
| private final String eventCreationDate; |
Maybe rename this variable so it's a little clearer? By its name only, it's not obvious that this variable ends up being a date and is set equal to DATE_FORMAT.format(new Date()) in the PerformanceEvent constructor. Even though String is next to the name, it sounds like it could represent some sort of boolean about whether the event is created or not.
There was a problem hiding this comment.
@langsmith I think this is what we use in other events . Also this matches the name on the backend so we do not have to add a serialization annotation.
|
@andrlee @langsmith I think the plan is to test it in the demo app. |
| * Customer measurements can be added to the bundle. | ||
| */ | ||
| public class PerformanceEvent extends Event { | ||
| private static final String PERFORMANCE_TRACE = "performance.trace"; |
added performance event for measuring latency and tracing download failures