Adding ResetLogSessionData method to reset session Id on session end

[kivarsha]

Related to the previous PR #677, resetting the timer was useful, but the sessionId also has to change. In order to reset the session data, this change exposes a new method on the Logger and LogManager called ResetLogSessionData which discards and re-creates the session data, creating a new sessionId for the new session within the same SDK lifecycle.

[maxgolov]

I think it's not a bad idea. My worry is mostly the projections to all other languages. I'll approve, but please wait for at least one other approval on this.

[maxgolov]

Actually - another alternate thought, since we have LogSessionData class, would it make more practical sense to have some form of reset method on it instead? Like this getter: https://github.com/microsoft/cpp_client_telemetry/blob/9310e6303c1738905c9fc3bf92507e929648721c/lib/include/public/LogManagerBase.hpp#L641

Maybe you use this to get the data, then reset data instead of adding a separate high-level API for that? What are your thoughts about this approach? Basically keeping the impl internally, but external API - bind thru the LogSessionData object rather than thru the LogManager object.

[maxgolov]

LogSessionData->reset() ? It might be also related to some changes that @lalitb has been working on, to clean up the state vars in case if user exercises the right to be forgotten.

[kivarsha]

That makes sense. We might also shift on this logic completely - if this is being used only by our team, we might shift this functionality into the client app itself, as that gives us more fine grained control over session telemetry, and I'll patch up this PR or abandon it based on how viable that approach is.

[larvacea]

I do believe that application teams are in the best position to define "session," based on their application's behavior on each platform.

[maxgolov]

I think adding reset is reasonable. @lalitb - could you please check and comment, how / if that intersects with your other privacy-related PR for Edge?

[maxgolov]

@lalitb

[lalitb]

I think adding reset is reasonable. @lalitb - could you please check and comment, how / if that intersects with your other privacy-related PR for Edge?

@maxgolov - To me, LogSessionData represent the logical data, and any changes to this data should come from LogSessionDataProvider. Resetting LogSessionData directly would cause discrepancy between session values in session-file and LogSessionData. I know this won't cause any problem as we won't be accessing the file again after session is initialized, but still I feel it's better to maintain the consistency.
This change looks good to me, maybe if we want to improve it, we can have LogSessionDataProvider::ResetLogSessionData() to actually reset the session, instead of deleting the file first and then recreating.

[maxgolov]

LogSessionDataProvider::ResetLogSessionData()

Yes, and probably only expose it as LogManager::ResetLogSessionData() ... maybe ResetSessionData() .

@kivarsha - Not sure we want this at ILogger level since it ends up calling the LogManager anyways?

[kivarsha]

@lalitb, I'm not sure how to cleanly fold the reset functionality without deleting/creating. Fundamentally, what needs to be done is a reset of the sessionId, however, that change would need to still be written to either db or file, so in essence, the same work would need to be done - deleting the existing source of truth, and generating a new one based on the same prior information (first launch date). In my last iteration, I pushed down the deletion/creation to the LogSessionDataProvider to have a clear separation of concerns. I'm not sure of a good way to simplify that logic and still retain the correct file/db record.

[maxgolov]

It looks good to me. We can tweak / refactor, and if we do - we'll notify you @kivarsha . Just clarify if you are planning to use it or not. This seems like a good addition.

[kivarsha]

@maxgolov, the current plan is to implement this on the client side, and I have the PRs out for that. This way, we get rid of our custom Outlook Mobile Android branch (outlookmobile/droid_rebased) where we also make some adjustments to setting the session Id + it allows us to have more control over rollout/implementation without having to wait for 1DS Pods.

[maxgolov]

the current plan is to implement this on the client side

Makes sense. That'll also live us some headroom to refactor this if needed. Thank you for your contribution! I think it's a good feature others would find useful.

[maxgolov]

There is some totally odd failure in Gtest on Mac. Could be some update to Mac OS X toolchain in GitHub action. I have some thoughts how to fix this..

[maxgolov]

711 - looks like GTest is now busted on Mac (just literally worked last night Okay).

[maxgolov]

Fixed the GTest failure. Updated your PR. Let's see ( fingers crossed ).

[maxgolov]

@kivarsha - it is now all good, GTest failure is gone, you can merge.