1432955 - Add telemetry for Rust SDP Parser

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P3)

Description

[Dan Minor [:dminor]]

We'll likely want to add telemetry that compares the results of the sipcc and rust SDP parser.

Comment 1

[Johannes Willbold]

Attachment: Bug 1432955: Implemented sdp parsing comparer,

Added signaling/src/sdp/ParsingResultComparer.h/.cpp
Added SDP comparer with telemetry and debug logging

Review commit: https://reviewboard.mozilla.org/r/259140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259140/

Comment 2

[François Marier [:francois]]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266188

Please include in the description what the key consists of.

Also, reading the description, it's not clear to me what you are recording. What kinds of differences are you recording?

::: toolkit/components/telemetry/Scalars.yaml:736
(Diff revision 1)
> +webrtc.sdp:
> +  parser_diff:
> +    bug_numbers:
> +      - 1432955
> +    description: >
> +      The differnces between the C based sipcc SDP parser and

typo: differences

::: toolkit/components/telemetry/Scalars.yaml:737
(Diff revision 1)
> +  parser_diff:
> +    bug_numbers:
> +      - 1432955
> +    description: >
> +      The differnces between the C based sipcc SDP parser and
> +      the new rust based rsdparsa SDP praser. This should help

typo: parser

Comment 3

[François Marier [:francois]]

Can you also please fill out a data review request form (https://github.com/mozilla/data-review/blob/master/request.md) and attach it as a .txt file to this bug? You can r? me on it and I will do the data review.

Comment 4

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259140/diff/1-2/

Comment 5

[François Marier [:francois]]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266206

Scalars.yaml looks good to me.

Comment 6

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #5)
> Comment on attachment 8994608 [details]
> Bug 1432955: Implemented sdp parsing comparer,
> 
> https://reviewboard.mozilla.org/r/259140/#review266206
> 
> Scalars.yaml looks good to me.

Just noting that this is not yet a datareview+, just an r+ on the Scalars.yaml changes.

Comment 7

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266206

The telemetry probes in ParsingResultComparer.cpp as well?

Comment 8

[François Marier [:francois]]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266206

I'll let the other reviewer look at the code changes. Data stewards generally only review documentation and probe metadata.

Comment 9

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266206

Alright, thank you.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266402

I'll give this one more look once you're done updating the changeset.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.h:19
(Diff revision 2)
> +
> +class ParsingResultComparer
> +{
> +public:
> +
> +  using AttributeType = SdpAttribute::AttributeType;

Let's put this in the cpp file. This would also let us forward declare things like Sdp, SdpMediaSection, and SdpAttributeList. We will need to include <string> though.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.h:23
(Diff revision 2)
> +
> +  using AttributeType = SdpAttribute::AttributeType;
> +
> +  ParsingResultComparer() = default;
> +
> +  bool Compare(Sdp* rsdparsaSdp, Sdp* sipccSdp, std::string originalSdp);

Let's pass const references for the params here.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.h:24
(Diff revision 2)
> +  bool CompareMediaSections(const SdpMediaSection& rustMediaSection,
> +                            const SdpMediaSection& sipccMediaSection);
> +  bool CompareAttrLists(const SdpAttributeList& rustAttrlist,
> +                        const SdpAttributeList& sipccAttrlist,
> +                        int level);

Should these functions be const?

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.h:34
(Diff revision 2)
> +
> +private:
> +
> +  std::string mOriginalSdp;
> +
> +  std::string GetAttributeLines(std::string attrType, int level) const;

Let's pass a const reference here.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:28
(Diff revision 2)
> +  std::stringbuf sb;
> +  std::ostream os(&sb);

Let's use ostringstream here.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:48
(Diff revision 2)
> +
> +  if (rsdparsaSdp->ToString() == sipccSdp->ToString()) {
> +    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
> +                         NS_LITERAL_STRING("serialization_is_equal"), 1);
> +    LOGD(("Serialization is equal"));
> +    return result;

Just return true here.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:53
(Diff revision 2)
> +  LOGD(("Serialization is not equal\n"
> +        " --- Sipcc SDP ---\n"
> +        "%s\n"
> +        "--- Rsdparsa SDP ---\n"
> +        "%s\n",
> +        sipccSdp->ToString().c_str(),
> +        rsdparsaSdp->ToString().c_str()));

Let's save the string representations in a local variable instead of re-serializing, to avoid confusing logging if there is some intermittent bug in serialization.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:65
(Diff revision 2)
> +    LOGD(("origin is not euqal\nrust origin: %ssipcc origin: %s",
> +          ToString(rsdparsaSdp->GetOrigin()).c_str(),
> +          ToString(sipccSdp->GetOrigin()).c_str()));

Similar thing here.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:65
(Diff revision 2)
> +
> +  // Compare the session level
> +  if (ToString(rsdparsaSdp->GetOrigin()) != ToString(sipccSdp->GetOrigin())) {
> +    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
> +                         NS_LITERAL_STRING("o="), 1);
> +    LOGD(("origin is not euqal\nrust origin: %ssipcc origin: %s",

s/euqal/equal/ over this whole file.

Also, we probably want some whitespace between the "%s" and the "sipcc" in this format string.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:68
(Diff revision 2)
> +    Telemetry::ScalarAdd(Telemetry::ScalarID::WEBRTC_SDP_PARSER_DIFF,
> +                         NS_LITERAL_STRING("o="), 1);
> +    LOGD(("origin is not euqal\nrust origin: %ssipcc origin: %s",
> +          ToString(rsdparsaSdp->GetOrigin()).c_str(),
> +          ToString(sipccSdp->GetOrigin()).c_str()));
> +  result = false;

Nit: fix indent.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:76
(Diff revision 2)
> +      LOGD(("Session level attribute count is equal, count: %u",
> +            rsdparsaSdp->GetAttributeList().Count()));
> +    } else {
> +      LOGD(("Session level attribute count is NOT equal, rsdparsa: %u, "
> +            "sipccs: %u\n",
> +            rsdparsaSdp->GetAttributeList().Count(),
> +            sipccSdp->GetAttributeList().Count()));

s/sipccs/sipcc/ over this whole file.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:76
(Diff revision 2)
> +      LOGD(("Session level attribute count is equal, count: %u",
> +            rsdparsaSdp->GetAttributeList().Count()));

Once we've determined that something doesn't match, let's focus on logging just the failures.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:100
(Diff revision 2)
> +    result &= CompareMediaSections(rsdparsaSdp->GetMediaSection(i),
> +                                   sipccSdp->GetMediaSection(i));

This is going to crash if there are fewer rust m-sections than sipcc m-sections.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:114
(Diff revision 2)
> +                                                          rustMediaSection,
> +                                            const SdpMediaSection&
> +                                                          sipccMediaSection)
> +{
> +  bool result = true;
> +  auto trackMediaLineValues = [&result] (auto rustValue, auto sipccValue,

Let's call this trackMediaLineMismatch.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:157
(Diff revision 2)
> +  compareSimpleMediaLineValue(&SdpMediaSection::GetProtocol,
> +                              NS_LITERAL_STRING("protocol"));
> +  compareSimpleMediaLineValue(&SdpMediaSection::IsReceiving,
> +                              NS_LITERAL_STRING("is_receiving"));
> +  compareSimpleMediaLineValue(&SdpMediaSection::IsSending,
> +                              NS_LITERAL_STRING("is_sendig"));

s/sendig/sending/

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:188
(Diff revision 2)
> +    // This was checked already
> +    if (type == AttributeType::kDirectionAttribute) {
> +      continue ;
> +    }

This is not checked for the session level. Let's do the direction checking down here, and remove the check from earlier.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:193
(Diff revision 2)
> +    // This was checked already
> +    if (type == AttributeType::kDirectionAttribute) {
> +      continue ;
> +    }
> +
> +    if (sipccAttrlist.HasAttribute(type, false)) {

I don't see anything that checks for the rust parser finding an attribute that sipcc did not. Do we intend to not treat this as an error? If we're going to let this slide, we probably want to avoid the whole-SDP serialization checking, and also not complain when the attr list counts differ.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:200
(Diff revision 2)
> +      attrStr += SdpAttribute::GetAttributeTypeString(type);
> +
> +      if (!rustAttrlist.HasAttribute(type, false)) {
> +        nsString typeStr;
> +        typeStr.AssignASCII(attrStr.c_str());
> +        typeStr += NS_LITERAL_STRING("_misses");

s/misses/missing/

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:212
(Diff revision 2)
> +        result = false;
> +        continue;
> +      }
> +
> +      auto rustAttrStr = ToString(*rustAttrlist.GetAttribute(type, false));
> +      auto sipccAttrStr = ToString(*sipccAttrlist.GetAttribute(type, false));

Maybe move |sipccAttrString| up so we can use it for the logging above?

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:264
(Diff revision 2)
> +  std::vector<std::string> lines = SplitLines(mOriginalSdp);
> +  std::string attrToFind = attrType + ":";
> +  std::string attrLines;
> +  int currentLevel = -1;
> +  // Filters rtcp-fb lines that contain "x-..." types
> +  // for exmaple: a=rtcp-fb:121 x-foo

s/exmaple/example/

Also, do we think this will happen particularly often? There are bunches of other cases where sipcc (and rust) ignore stuff that is not supported, is this one special in some way?

Comment 11

[Johannes Willbold]

Attachment: data collection request form

Comment 12

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259140/diff/2-3/

Comment 13

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266402

> s/exmaple/example/
> 
> Also, do we think this will happen particularly often? There are bunches of other cases where sipcc (and rust) ignore stuff that is not supported, is this one special in some way?

Yes, every SDP from Edge contains these rtcp-fb paremters.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266684

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.h:10
(Diff revisions 1 - 3)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef _PARSINGRESULTCOMPARER_H_
>  #define _PARSINGRESULTCOMPARER_H_
>  
>  #include "signaling/src/sdp/Sdp.h"

Can we avoid this include, and do forward declarations instead?

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.h:24
(Diff revisions 1 - 3)
>  
> -  using AttributeType = SdpAttribute::AttributeType;
> -
>    ParsingResultComparer() = default;
>  
> -  bool Compare(Sdp* rsdparsaSdp, Sdp* sipccSdp, std::string originalSdp);
> +  bool Compare(const Sdp* rsdparsaSdp, const Sdp* sipccSdp,

Let's use references here instead of pointers.

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:234
(Diff revisions 1 - 3)
>          } else {
> -          LOGD(("But the rust serialization is euqal to the orignal sdp\n"));
> +          LOGD(("But the rust serialization is equal to the orignal sdp\n"));
> +        }
> -        }
> +      }
> +    } else {
> +      if (!rustAttrlist.HasAttribute(type, false)) {

I think you need to omit the '!', right?

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:237
(Diff revisions 1 - 3)
> -        }
> +      }
> +    } else {
> +      if (!rustAttrlist.HasAttribute(type, false)) {
> +        nsString typeStr;
> +        typeStr.AssignASCII(attrStr.c_str());
> +        typeStr += NS_LITERAL_STRING("_exclusive");

Maybe s/exclusive/unexpected/ ?

::: media/webrtc/signaling/src/sdp/ParsingResultComparer.cpp:269
(Diff revisions 1 - 3)
>    // Filters rtcp-fb lines that contain "x-..." types
> -  // for exmaple: a=rtcp-fb:121 x-foo
> +  // for example: a=rtcp-fb:121 x-foo

Ok, let's mention that we're doing this because edge uses these, making it a common case.

Comment 15

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

https://reviewboard.mozilla.org/r/259140/#review266684

> I think you need to omit the '!', right?

Right

Comment 16

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259140/diff/3-4/

Comment 17

[Johannes Willbold]

Comment on attachment 8994608 [details]
Bug 1432955: Implemented sdp parsing comparer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259140/diff/4-5/

Comment 18

[François Marier [:francois]]

Comment on attachment 8994942 [details]
data collection request form

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, Scalars.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default ON in pre-release channels only.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts are enough.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76a4475d0523
Implemented sdp parsing comparer, r=bwc,francois

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/76a4475d0523