microsoft / cpp_client_telemetry

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

Invalid event type in error logs #679

Closed suryatn closed 3 years ago

suryatn commented 3 years ago

By not setting eventType, I am getting the following messages in error logs.

2020-10-17 20:43:22.067 7043-7043/com.microsoft.skype.teams.dev E/MATSDK: Invalid event name - "": must be between 4 and 100 characters long
2020-10-17 20:43:22.067 7043-7043/com.microsoft.skype.teams.dev E/MATSDK: Invalid event type!

So to avoid these errors, we are setting eventType same as eventName. I believe Eric from OneDrive team is also handling in the same way.

I think the error is from the below code. https://github.com/microsoft/cpp_client_telemetry/blob/master/lib/system/EventProperties.cpp#L282-L300

setType might be called internally here https://github.com/microsoft/cpp_client_telemetry/blob/master/lib/jni/JniConvertors.cpp#L157-L179

Attaching .saz file from fiddler.

1ds.saz.zip

@maxgolov @mkoscumb @larvacea @sid-dahiya can we please check this once?

maxgolov commented 3 years ago

SetType API call is optional in C++. What happens if you run a simple Hello World sample on Desktop without Java / JNI projection? It will work fine. If JNI is unconditionally calling it - even with an empty string (please check this), then a fix would be to not call SetType from Java layer if an empty string is provided / if type is not set.

maxgolov commented 3 years ago

You would not see any issue in Fiddler since this is informational error that suggests that some other higher level code (user code or JNI bindings) are calling into native C++ API with invalid input.

suryatn commented 3 years ago

@saurabhagr12 I think the issue could be in the JNI code as per Max mentioned above. Can we please check so that we can have the fix from next version onwards. Thanks.

maxgolov commented 3 years ago

@saurabhagr12 @suryatn - please check, guys, most likely it's the JNI layer. Feel free to assign this to yourself and send in a PR.

saurabh-sagrawal commented 3 years ago

@suryatn can you please verify this by doing the empty string check before calling SetType method in the jni code since you already have the repro available with you.

suryatn commented 3 years ago

@saurabhagr12 currently it would be difficult for me to check as I am handling release for Teams app and also am on-call for the next week. If possible please take a look when you are free. Thanks.

maxgolov commented 3 years ago

@suryatn @saurabhagr12 - what was the outcome of your investigation, guys? Can this issue be closed?

suryatn commented 3 years ago

@saurabhagr12 this can be an issue in the JNI layer, can you please check once and we can have the fix from next version onwards if this is actually an issue. Thanks

saurabh-sagrawal commented 3 years ago

@suryatn I need some help with the repro here for the POC. What APIs are you using and what values did you try when you see this error.

suryatn commented 3 years ago

@saurabhagr12 seems like all the APIs like logEvent(), logAppLifecycle(), logTrace() etc call getEventProperties in Logger_jni.cpp here https://github.com/microsoft/cpp_client_telemetry/blob/master/lib/jni/Logger_jni.cpp#L179-L554

getEventProperties() calls setType() here https://github.com/microsoft/cpp_client_telemetry/blob/master/lib/jni/JniConvertors.cpp#L157-L179

and we might be getting error from here https://github.com/microsoft/cpp_client_telemetry/blob/master/lib/system/EventProperties.cpp#L282-L300

Please see if we can make getEventProperties() not call setType() when type is empty.

maxgolov commented 3 years ago

@suryatn @saurabhagr12 @larvacea - do you guys have plans to fix it? I have seen another customer wondering about this bogus warning message.

suryatn commented 3 years ago

@maxgolov - @saurabhagr12 @larvacea should be able to provide an update on it. @saurabhagr12 please let us know if you were able to resolve the above error. Thanks.

saurabh-sagrawal commented 3 years ago

Assigned to self for fixing this. @maxgolov, @suryatn how do we enable and access the debug logs.

suryatn commented 3 years ago

@saurabhagr12 you can set LogConfigurationKey.CFG_INT_TRACE_LEVEL_MIN log configuration to 0L to access debug logs.