1856644 - Using a debug tag with a whitespace silently fails

Status: RESOLVED INVALID

(Data Platform and Tools :: Glean: SDK, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

While debugging an issue with the Glean Debug View, I found that when setting a debug tag on Android using Glean.setDebugViewTag("my tag") (notice the whitespace), the tag doesn't get set or the data simply won't show on the debug view.

If the tag is somehow in an unexpected format, we should:

1. document the expected format
2. reject the tag with an error

Comment 1

[Jan-Erik Rediger [:janerik]]

We log an error! But yes, it is indeed suboptimal. There's more restrictions: only ascii letters and digits and -, no more than 20 characters.
The API to set the tag returns a boolean flag on whether the debug tag was accepted. It always returns true if called before initialize.

"reject the tag with an error" -- what do you expect here to happen? How do we communicate this to the user?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #1)

We log an error!

Interesting, we were live debugging in Android Studio and both did not see it.

"reject the tag with an error" -- what do you expect here to happen? How do we communicate this to the user?

One path is to call out in the docs that:

• To make sure the API call was successful the return value must be checked (not sure if we can do that programmatically with some kotlin annotation?)
• What happens if this is called before init

As for the expectation, I'd expect the function to fail validation even if called before init, likely here? I'm not sure where we're doing the validation, though

Comment 3

[Jan-Erik Rediger [:janerik]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)

(In reply to Jan-Erik Rediger [:janerik] from comment #1)

We log an error!

Interesting, we were live debugging in Android Studio and both did not see it.

Hm, I guess Glean logs are not enabled by default (anymore).

"reject the tag with an error" -- what do you expect here to happen? How do we communicate this to the user?

One path is to call out in the docs that:

• To make sure the API call was successful the return value must be checked (not sure if we can do that programmatically with some kotlin annotation?)

The Kotlin side could check it and log (or explode or something)

• What happens if this is called before init

We stuff away the value and apply it after init. That's why we don't check validity at that time. It's an unfortunate side-effect of how the API is designed.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #3)

(In reply to Alessio Placitelli [:Dexter] from comment #2)

(In reply to Jan-Erik Rediger [:janerik] from comment #1)

We log an error!

Interesting, we were live debugging in Android Studio and both did not see it.

Hm, I guess Glean logs are not enabled by default (anymore).

Ouch. Maybe we should still log from the debugging APIs even if log is disabled?

• To make sure the API call was successful the return value must be checked (not sure if we can do that programmatically with some kotlin annotation?)

The Kotlin side could check it and log (or explode or something)

That feels reasonable!

Comment 5

[Jan-Erik Rediger [:janerik]]

I briefly looked into this:

• setDebugViewTag is not a public API in Kotlin, so how did you call it directly?
• We do log an error when validation fails.
• Spaces in the tag fail the validation
• log::error logs are visible in logcat
• I ran adb shell am start -n org.mozilla.fenix/mozilla.telemetry.glean.debug.GleanDebugActivity --ez logPings true --es sendPing baseline --es debugViewTag 'janerik space test'. This interestingly enough send a ping tagged janerik. I think am is swallowing the spaced-off part somewhere?
• Passing 'janerik space test' correctly logs:

libglean_ffi  E  glean_core::debug: Invalid character ' ' in the tag.
              E  glean_core::debug: Invalid value for debug option GLEAN_DEBUG_VIEW_TAG.

• Invoking adb shell and in there using am start ... --es debugViewTag 'janerik space test' also does the expect thing: we get the error in the log.

Conclusion: String interpolation across shells is hard. And adb is bad at that.
All in all Glean is already doing its best though and logging when the validation fails, thus informing the user of the error.

I think we can safely close this.

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #5)

I briefly looked into this:

• setDebugViewTag is not a public API in Kotlin, so how did you call it directly?

We just called Glean.setDebugViewTag. We were usign the GleanSDK directly outside of A-C.

• We do log an error when validation fails.
• Spaces in the tag fail the validation
• log::error logs are visible in logcat

Odd, we tried but didn't see anything. I guess it was not showing up for other reasons (although we could see other glean logs).