microsoft / cpp_client_telemetry

1DS C++ SDK
Apache License 2.0
85 stars 48 forks source link

Query on SessionId #743

Closed suryatn closed 3 years ago

suryatn commented 3 years ago

When testing @larvacea's multiple log manager changes (pulling the recent master 03e246e4c0955e5f8707399f046a1a2212a92bb2), I observed that sessionId is getting logged as different for logSession() API.

As suggested earlier from your team, we are generating SessionId using UUID.randomUUID().toString() on our client during app start only once and setting it as global context using LogManager's setContext(key, value, getOneDSPiiKind(PiiKind.NONE)) method. We are setting the same sessionId even on cloud switch scenario. E.g sessionId generated on client = 591e1f85-c848-4ac4-b352-96e669a29d89 and this value gets appended to all events. However for session table using logSession API, the session_Id seems to be different 09c9dc06-7c24-dae8-d0f8-bfa4dacab128. Please help in resolving this issue.

@maxgolov @larvacea @mkoscumb @sid-dahiya

maxgolov commented 3 years ago

@lalitb may elaborate, as he did some recent refactor related to this.

I believe what you are observing is design intent:

If you don't like this built-in behavior, then don't use LogSession API: use LogEvent + SetContext. You may be able to decorate your event manually..

There might be some way to handle it differently. Here:

https://github.com/microsoft/cpp_client_telemetry/blob/1f2c4d0eb721910fa3e2d9557d6d537c964f9d66/lib/decorators/SemanticApiDecorators.hpp#L283

I see that the code here is doing setIfNotEmpty for the Session ID.. Try to set an event property of your LogSession API event SESSION_SDKUID="DeviceInfo.SDKUid" to the value you set in setContext. That should let you override the value.

larvacea commented 3 years ago

On MacOS, unit tests still create a session file, though that doesn't prove that the SDK actually stores anything there.

On Android, I do not see a session file, so I would assume that the SDK is using the database and storing the session ID in the key-value table. I will add unit tests if we don't have them already, because why not actually test this and prevent regressions?

maxgolov commented 3 years ago

@lalitb - since you recently refactored this code, could you clarify how it is expected to work?

suryatn commented 3 years ago

I see that the code here is doing setIfNotEmpty for the Session ID.. Try to set an event property of your LogSession API event SESSION_SDKUID="DeviceInfo.SDKUid" to the value you set in setContext. That should let you override the value.

@maxgolov @larvacea @lalitb currently we are are using logSession API using below code

String sessionId = UUID.randomUUID().toString();
oneDSLogManager.setContext("Session.Id", sessionId);
EventProperties sessionEventProperties = new EventProperties("session");
logSession(SessionState.Started, sessionEventProperties);

How can we make sure that the sessionId is same as the generated one for logSession API for different logManagers (i.e different clouds). Will setting "DeviceInfo.SDKUid" to generated sessionId help in achieving the above using

sessionEventProperties.setProperty("DeviceInfo.SDKUid", sessionId);
logSession(SessionState.Started, sessionEventProperties)

@maxgolov can you please help explain why setting "DeviceInfo.SDKUid" will help in using the custom generated sessionId instead of SDK sessionId? I will also try to validate locally if it works.

maxgolov commented 3 years ago

@suryatn

sessionEventProperties.setProperty("DeviceInfo.SDKUid", sessionId);
logSession(SessionState.Started, sessionEventProperties);

should work. Test it. Set a breakpoint to decorator that I showed you above and see if it respects the value, if already set.

How can we make sure that the sessionId is same as the generated one for logSession API for different logManagers

You just don't: you override it with your own session ID that you acquire from elsewhere. @laltib can help you clarify on this - maybe you can retrieve the 1st log manager UUID, then stamp it on 2nd instance using the property method above + stamp it on all other events using setContext API?

suryatn commented 3 years ago

@maxgolov sure will try. Not sure how we can apply a break point on cpp code (as we consume SDK via AAR), will check on kusto once. For sessionId we generate only once and store it as a static variable, so stamping it using above method + on all other events could work.

suryatn commented 3 years ago

@maxgolov just to confirm, we need to override DeviceInfo.SDKUid property?, We use Session_Id as the field for doing joins, so we would need Session_Id to be same for all the events including the event logged via logSession API. We are already setting custom sessionId using logManager context. I am also trying the above change suggested by you.

maxgolov commented 3 years ago

Actually you're right:

https://github.com/microsoft/cpp_client_telemetry/blob/1f2c4d0eb721910fa3e2d9557d6d537c964f9d66/lib/decorators/SemanticApiDecorators.hpp#L281

Note you'd better use this constant: https://github.com/microsoft/cpp_client_telemetry/blob/1f2c4d0eb721910fa3e2d9557d6d537c964f9d66/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/Constants.java#L63

So final code to override your SESSION_ID should be:

sessionEventProperties.setProperty(SESSION_ID, sessionId);
logSession(SessionState.Started, sessionEventProperties);

I assume that you do not run analytics on DeviceInfo.SDKUid? Obviously it'd be also different for different LogManager instances.. as it's now read from DB. Since DB is unique per instance in multiple Log Manager scenarios - each LogManager would have its own DeviceInfo.SDKUid.

Please try it out.

suryatn commented 3 years ago

@maxgolov @lalitb I have tried to set Session.Id as event property, however for session event, the session_Id field is still different from other events. I have checked in Aria/Kusto. I have used the below code.

String sessionId = UUID.randomUUID().toString();
oneDSLogManager.setContext("Session.Id", sessionId);
EventProperties sessionEventProperties = new EventProperties("session");
sessionEventProperties.setProperty("Session.Id", sessionId);
logSession(SessionState.Started, sessionEventProperties);

Please let me know how we can resolve this issue.

@maxgolov @larvacea Also couple of questions on multiple log manager: (slightly different from session topic)

  1. Currently we are not setting LogConfigurationKey.CFG_INT_MAX_TEARDOWN_TIME to config (It should continue to use the default value I assume) as we are not doing any teardown during cloud switch. The cloud switch is working fine even without setting this config. Can you please confirm if that is fine?
  2. In case of multiple log manager, we are not doing any init and directly creating logManager using logMangerProvider for all cloud types. For offline storage upload events as per Martin: When you create the ILogManager (LogManagerImpl or LogManagerBase, I don't recall offhand which is the actual instantiated C++ class), it will have its own database and tpm (transmit policy manager) components, and it will take any events it finds in its database (as set by the config) and upload them on its configured schedule. Can you please point to the code just to confirm where the upload happens on creating logManager. We would want to make sure that offline events are getting uploaded on app re-launch (i.e on creating a new logManager).
maxgolov commented 3 years ago

It would make sense to track your questions in Teams chat. Let's keep focused on original problem. Answers:

Here:

https://github.com/microsoft/cpp_client_telemetry/blob/a6e0e8b7fe61b03fdf231a7d956db97eca7cbd86/lib/system/TelemetrySystem.cpp#L43

Then here:

https://github.com/microsoft/cpp_client_telemetry/blob/a6e0e8b7fe61b03fdf231a7d956db97eca7cbd86/lib/system/TelemetrySystem.cpp#L47

Then, finally, here:

https://github.com/microsoft/cpp_client_telemetry/blob/a6e0e8b7fe61b03fdf231a7d956db97eca7cbd86/lib/tpm/TransmissionPolicyManager.cpp#L219

suryatn commented 3 years ago

Thanks @maxgolov. Just to confirm TelemetrySystems gets triggered on creating every logManager right?

Also please let me know if there is way to make the session.Id property in logSession API same as the one in other events using the above mentioned code snippet. We would want that Session_Id field in session table to be same as the one in other tables.

maxgolov commented 3 years ago

@suryatn - yes, instance of TelemetrySystem is created for each host LogManager.

Please test your code change as above -- setting Session.Id property, then capture Fiddler trace, and attach it to PR. @agavipul knows how to capture the trace. This is needed since you can't set a breakpoint in C++ code -- we need a trace to confirm this field contents is not something that is altered / modified by collector server... Otherwise if you can debug / set a breakpoint in C++, set it and inspect the record contents at this line - after the decorator is done decorating.

https://github.com/microsoft/cpp_client_telemetry/blob/aa97b7cd3997b47b15ec4975eab1bf593411f311/lib/api/Logger.cpp#L852

maxgolov commented 3 years ago

@suryatn - I think I did not interpret the code correctly.

Currently there is no mechanism to override the SDK-managed Session.Id property, specifically if LogSession API is used. Your workaround is to use LogEvent and manually populate the identical set of fields.

Session.Id is autogenerated each time here in LogSession method: https://github.com/microsoft/cpp_client_telemetry/blob/c3b1cb615f7c75f924f835c5ff97b459f45a918c/lib/api/Logger.cpp#L819

Each LogManager instance has its own telemetry system, storage, upload cadence, and its own Session.Id. When you start another session, it is treated as new session for the LogManager and is NOT presently overridable.

One way of fixing this is if you contribute a PR that checks if Session.Id is provided inside the EventProperties collection, i.e. if this key exists in properties, then use its value instead of auto-generating the new GUID.

This parameter: https://github.com/microsoft/cpp_client_telemetry/blob/c3b1cb615f7c75f924f835c5ff97b459f45a918c/lib/api/Logger.cpp#L767

And add the check here: https://github.com/microsoft/cpp_client_telemetry/blob/c3b1cb615f7c75f924f835c5ff97b459f45a918c/lib/api/Logger.cpp#L819

If you need this feature, feel free to contribute a PR. I will assign it to you if you don't mind.

suryatn commented 3 years ago

@maxgolov currently in session event table, the properties related to session that we mainly use are below

"Session_Id": 72f8f785-1f8b-9059-5920-cceb8ae2880f,
"Session_State": Started,

As suggested by you, we can try to use LogEvent for session event and add the above fields as event properties. Will check once if it working fine. We can make the change in C++ side if it is needed later. Please confirm if that would be fine.

maxgolov commented 3 years ago

As suggested by you, we can try to use LogEvent for session event and add the above fields as event properties

This approach should work. Make sure you set the same properties, with your own Session.Id.

We can make the change in C++ side if it is needed later

Makes sense.

maxgolov commented 3 years ago

I'm closing this issue, since I believe the question has been answered. Please reopen it if it's still a concern.