microsoft / cpp_client_telemetry

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

Need information on Diagnostic Level #610

Closed suryatn closed 3 years ago

suryatn commented 3 years ago

Why do we need to set Diagnostic Level for an EventProperties? By default, it picks up optional value. How does 1DS and DDV use that information? We already have some classification based on user/event level privacy data control configuration for every event. We set them as a property in EventProperties.

@maxgolov @sid-dahiya

sid-dahiya commented 3 years ago

They're required by DDV for identifying events and by the built in filtering. See docs/Privacy-Settings.md

@mkoscumb can share details about the filtering.

suryatn commented 3 years ago

@sid-dahiya Do we need to set PrivacyDiagnosticTag as well? Is it mandatory for 1DS/DDV?

sid-dahiya commented 3 years ago

Yes, both PDT and PDL are required for DDV support. Without them, DDV does not know how to process the data for viewing support.

suryatn commented 3 years ago

@sid-dahiya @maxgolov do we need to also set type for EventProperties, is it mandatory? What is the difference between eventType and eventName? Currently setting eventType same as eventName.

maxgolov commented 3 years ago

You do not need to set type. It is only used in rare cases for Cosmos routing / by ODIN Data Cooker. If your project doesn't use it, you don't set it.

maxgolov commented 3 years ago

@suryatn - everything is described in the doc here: https://github.com/microsoft/cpp_client_telemetry/blob/master/docs/Privacy-Settings.md

Clarifications re. PDT:

For MS Teams being under the Office umbrella, you need to set both PDT and PDL. I'm closing this issue. Please follow-up with @sid-dahiya if you believe that some of these clarifications must be added to the Privacy-Settings.md doc.

suryatn commented 3 years ago

@sid-dahiya @maxgolov do we need to also set type for EventProperties, is it mandatory? What is the difference between eventType and eventName? Currently setting eventType same as eventName

You do not need to set type. It is only used in rare cases for Cosmos routing / by ODIN Data Cooker. If your project doesn't use it, you don't set it.

@maxgolov @sid-dahiya @mkoscumb @larvacea I am getting the below error if I am setting type. Is setting type mandatory? Please confirm as this is showing up as a lot of errors in the logcat. Do I need to set its value back to eventName?

2020-09-11 02:46:19.135 19658-19658/com.microsoft.skype.teams.dev E/MATSDK: Invalid event type!
sid-dahiya commented 3 years ago

Can you share a code snippet that's giving this error?

suryatn commented 3 years ago

@sid-dahiya I think the error is from the EventProperties.cpp

    /// <summary>
    /// Specify the Base Type of an event. This field is populated in Records.Type
    /// </summary>
    bool EventProperties::SetType(const string& recordType)
    {
        std::string eventType = toLower(recordType);
        eventType = sanitizeIdentifier(eventType);
        EventRejectedReason isValidEventName = validateEventName(eventType);
        if (isValidEventName != REJECTED_REASON_OK) {
            LOG_ERROR("Invalid event type!");
            DebugEvent evt;
            evt.type = DebugEventType::EVT_REJECTED;
            evt.param1 = isValidEventName;
            ILogManager::DispatchEventBroadcast(evt);
            return false;
        }
        m_storage->eventType.assign(eventType);
        return true;
    }
maxgolov commented 3 years ago

@suryatn - what are you setting type to (and why?)

sid-dahiya commented 3 years ago

@sid-dahiya I think the error is from the EventProperties.cpp


    /// <summary>

    /// Specify the Base Type of an event. This field is populated in Records.Type

    /// </summary>

    bool EventProperties::SetType(const string& recordType)

    {

        std::string eventType = toLower(recordType);

        eventType = sanitizeIdentifier(eventType);

        EventRejectedReason isValidEventName = validateEventName(eventType);

        if (isValidEventName != REJECTED_REASON_OK) {

            LOG_ERROR("Invalid event type!");

            DebugEvent evt;

            evt.type = DebugEventType::EVT_REJECTED;

            evt.param1 = isValidEventName;

            ILogManager::DispatchEventBroadcast(evt);

            return false;

        }

        m_storage->eventType.assign(eventType);

        return true;

    }

This method isn't related to anything with Privacy Data Types. @maxgolov what type is this method setting?

maxgolov commented 3 years ago

It has absolutely nothing to do with data types. Just to reiterate 😃 -- You do not need to set type. It is only used in rare cases for Cosmos routing / by ODIN Data Cooker. If your project doesn't use it, you don't set it.

maxgolov commented 3 years ago

This is my_whatever_data_type_that_ODIN_cooker_may_use_to_perform_some_special_cooking. I don't know what that is, as this is semantic convention hidden from naked 1DS / Common Schema eye, something that was very legacy and unique to Office only.

Before I tell you what this method is setting, would you mind sharing what are you passing to it exactly (...and why)? 😄 (you don't need to call it in the first place)... Most likely it should be alphanumeric and no dots in it nor any special chars.. Just don't use it if you don't need it. You don't need it..

suryatn commented 3 years ago

@maxgolov @sid-dahiya I am getting the error when I am not explicitly setting any type. I do not get the error if I set it with some value (I am using name value itself for type)

sid-dahiya commented 3 years ago

Can you share a code snippet of how you're instrumenting your events, including the part where you're calling SetType?

maxgolov commented 3 years ago

@suryatn - I'm confused, since you mentioned:

I am getting the below error if I am setting type

How did you manage to post events before - in the other thread where you are app is setting some invalid User ID?

Would you mind sharing your code sample for that exact event where you are observing an error? This error is actually synchronous: SetType returns false if it fails. You can debug the place where it happens.. Set a conditional break point. And you may also continue ingesting your event after that no problem.

suryatn commented 3 years ago

@sid-dahiya @maxgolov below is the sample snippet which I use

EventProperties eventProperties = new EventProperties("test event name");
eventProperties.setPrivacyMetadata(PrivacyDiagnosticTag.ProductAndServiceUsage, DiagnosticLevel.DIAG_LEVEL_REQUIRED);
eventProperties.setType(eventProperties.getName()); // this it the line I am adding to avoid the above error. If I am not setting type, it is resulting in error
mLogger.logEvent(eventProperties);

I will also try to check with debugger

sid-dahiya commented 3 years ago

@maxgolov To hold me honest, but I don't think you need that line. Even if you do, you're not setting a valid type, you're just setting the event name as type.

To reiterate Max's earlier question, why are you setting that type?

suryatn commented 3 years ago

@sid-dahiya I am setting type to prevent the error. I think oneDrive is using the same approach (not sure if they have changed it recently). If it can be resolved without that, I would prefer not to set type.

sid-dahiya commented 3 years ago

@sid-dahiya I am setting type to prevent the error. I think oneDrive is using the same approach (not sure if they have changed it recently). If it can be resolved without that, I would prefer not to set type.

What is an actual eventName that you are using? I am wondering if the error you're seeing is because the eventName is possibly invalid. For example test event name that you shared above is not a valid event name. https://github.com/microsoft/cpp_client_telemetry/blob/f5cb455e3550e9620a666be31ededbbd116ea3c8/lib/utils/Utils.cpp#L180

sid-dahiya commented 3 years ago

@maxgolov I'm reopening the issue as I'm a bit concerned there is some bug here.

suryatn commented 3 years ago

@sid-dahiya I am validating event name at my end as well before sending that follows the regex pattern, the earlier event is a sample event name.

maxgolov commented 3 years ago

@sid-dahiya @suryatn

I'm reopening the issue as I'm a bit concerned there is some bug here.

Please check if you can repro this with a simple Win32 Desktop app. If not, then I assume there's something else going on - unique to Android platform. SetType - is completely unrelated to original question on Diagnostic Levels. We can keep adding various questions to this thread, as long as they are relevant to original question. I believe SetType is not relevant. If you believe that there is an issue with SetType API - which you don't need to use, as it's only needed for ODIN Cooker and Geneva MDM - Metrics export of certain events, absolutely not set on all events, then please log an issue against the Android layer to investigate.

We do have a few spots in our sample app and tests that verify its behavior for the C++ layer. It works well for C++ layer.