Capture click events for compose#1002
Conversation
|
Thanks, looking forward to reviewing! One initial thought -- the new module is |
|
I like @breedx-splk's suggestion. Compose is an entire framework, and there could be instrumentation for many aspects of it. Having them live in separate modules seem like the right thing to do, as you might not want all of them. |
| autoService = "1.1.1" | ||
| androidx-navigation = "2.7.7" | ||
| #noinspection GradleDependency | ||
| compose = "1.3.0" |
There was a problem hiding this comment.
What's the reason you chose 1.3.0? Is it that you can only support versions at runtime that is greater than or equal to this version?
There was a problem hiding this comment.
What's the reason you chose 1.3.0?
1.3.0 because that's when the layoutDelegate field was added to LayoutNode class.
Is it that you can only support versions at runtime that is greater than or equal to this version?
yes
There was a problem hiding this comment.
I think there's no need to pin it to this version in the project, since we add it as compileOnly and the README explains that the minimum version required is 1.3.0. So I'd remove the #noinspection GradleDependency and set it to the latest version available.
| ctx.openTelemetry | ||
| .logsBridge | ||
| .loggerBuilder("io.opentelemetry.android.instrumentation.compose") | ||
| .build() as ExtendedLogger, |
There was a problem hiding this comment.
Will this work with disk buffering, @LikeTheSalad ?
There was a problem hiding this comment.
This is a good point. I'm not sure it would cause issues, but in general I don't think we should rely on extended types unless we have no choice. Can we change this to use plain Logger instead, @cleverchuk ?
There was a problem hiding this comment.
Unfortunately, the event name stuff is all incubating, which means that any time we want to create events we have to do the cast until it's done incubating. See the network change instrumentation for an existing precedent.
| import android.view.Window.Callback | ||
| import androidx.annotation.RequiresApi | ||
|
|
||
| internal class WindowCallbackWrapper( |
There was a problem hiding this comment.
Will this work with versions less than M if two of these methods are not callable? If the behavior in L is unpredictable as a result, I would just make the whole class require M and simply not add the instrumentation if it's not at least M
There was a problem hiding this comment.
this is similar to view-click one. IIRC animal-sniffer gave me a lot of grief when i made it class only and i think the tests didn't run correctly either with what you're suggesting.
There was a problem hiding this comment.
Tests that use the AndroidX or Roboletric runner will by default use the minApiVersion. You can use the @Config annotation to specify a working Android version and then test that a non-working one wouldn't have this added. Have a look at AgentInitTest among others.
There was a problem hiding this comment.
what are you suggesting, change the min api to M?
There was a problem hiding this comment.
I'm suggesting changing the tests to run as if it were M, and only including this instrumentation if it were M+.
No worries about this. I'll see if I can refactor and throw a test up quickly today since this was merged already
| val yCoordinateAttr: AttributeKey<Long> = longKey("app.screen.coordinate.y") | ||
| val viewIdAttr: AttributeKey<Long> = longKey("app.widget.id") | ||
|
|
||
| class ComposeLayoutNodeUtil { |
There was a problem hiding this comment.
Why not make the whole class internal so you don't have a public object with inaccessible methods outside the module?
There was a problem hiding this comment.
Right. It's because ComposeClickEventGenerator is public and this is part of the interface. Can that be internal as well? What you have here is a concept that is partially leaking publicly, but not really accessible which isn't great.
I think you have a few options:
- Make everything public
- Make everything internal
- Make the Util class completely private/internal and don't expose it in the interface of
ComposeClickEventGenerator
Any of these would work
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
This is great @cleverchuk thanks for the addition! I left a few comments, but things are mostly looking good from my perspective. Cheers!
|
I'm gonna close the PR and reopen it later because it looks like the compiler suppression doesn't work from 1.6.0+. |
|
okay, we can only claim to support 1.3.0 - 1.5.4. i'll work on figuring out how to support the rest. also moved to new package because it seemed like everyone liked the idea. |
4 participants
fixes #986
This PR takes inspiration from here and here to realize the feature.