DataDog / dd-sdk-flutter

Flutter bindings and tools for utilizing Datadog Mobile SDKs
Apache License 2.0
45 stars 42 forks source link

Add ability to add custom attributes to RumUserActionAnnotation #667

Open orevial opened 1 day ago

orevial commented 1 day ago

Feature description

Currently Datadog default implementation uses RumUserActionDetector to detect buttons & other actions taps. Also it gives RumUserActionAnnotation which allow providing custom description, to either override (for things like standard buttons or Semantics widgets) or implement tap-like strategies for custom elements.

When using RumUserActionAnnotation it would be great if we could also add an attributes map to this widget so we can further customize the tap action with custom attributes, just like we can do with DatadogSdk.instance.rum?.addAction().

Say I have an avatar image of a user, and the tap on this image will redirect to the user profile.

Currently what I can do is pass the user id and/or name as the action description, e.g.:

RumUserActionAnnotation(
  description: 'Avatar(user 123)'
  child: UserAvatar(),
)

However this is not ideal because it means that each avatar tap will result in a different action in Datadog. What I'd like to do instead is use a generic name as the description, and then use custom attributes to add additional data (e.g. for debugging purposes). Something along those lines:

RumUserActionAnnotation(
  description: 'Avatar(user)'
  attributes: {
    'userId': '123',
    'userName': 'John Doe',
  },
  child: UserAvatar(),
)

Proposed solution

@immutable
class RumUserActionAnnotation extends StatelessWidget {
  final String description;
  final Widget child;
  final Map<String, Object?> attributes;

  const RumUserActionAnnotation({
    super.key,
    required this.description,
    required this.child,
    this.attributes = const {},
  });

  @override
  Widget build(BuildContext context) {
    return child;
  }
}

Then in RumUserActionDetector we should check if attributes exist, and if they do we should give it to addAction method:

  void _onPerformActionAt(Offset position, RumActionType action) {
    final elementDescription = _getDetectingElementAtPosition(position);

    if (elementDescription != null) {
      widget.rum?.addAction(
         action, 
         elementDescription.toString(),
         // Also give the optional attributes
         attributes,
);
    }
  }

Other relevant information

No response

fuzzybinary commented 1 day ago

@orevial This looks good to me. Would you be up for implementing / submitting a PR?

orevial commented 1 day ago

Yes I think so, I will try to submit a PR in the next few days šŸ™‚

orevial commented 22 hours ago

Hey @fuzzybinary, I have another feature request that might be related. Do you think it would be possible to configure the RumUserActionAnnotation to replace the elementName below ?

My "issue" is that if I do that:

RumUserActionAnnotation(
  description: 'Avatar(user)'
  child: UserAvatar(),
)

And that my UserAvatar uses a GestureDetector or an InkWell below, the generated log will be GestureDetector(Avatar(user)) or InkWell(Avatar(user)). In this case I'd like to only keep Avatar(user) as the GestureDetector or InkWell do not bring any value to the action log.

What I'd like to do is something like this :

RumUserActionAnnotation(
  description: 'user'
  // šŸ‘‡ new param here
  overrideElementName: 'Avatar',
  child: UserAvatar(),
)

That would just generate Avatar(user).

What do you think, would that make sense ? If it does, should I try to include this in the same PR or do a different PR for this ?

fuzzybinary commented 19 hours ago

Since RumUserActionAnnotation overrides its entire tree, I'm worried that having an element rename could disguise where the gesture is actually being detected, and lead to some confusion in deeper trees.

One possible solution instead is to add a method that can give RUM information about a custom detection widget, and call that custom method first in this method. Method signature would be:

@immutable
class RumGestureDetectorInfo {
  final String elementName;
  final bool searchForBetter;
  final String searchForText;

  RumGestureDetectorInfo(this.elementName, {this.searchForBetter = false, this.searchForText = true});
}

typedef CustomGestureElementDetector = RumGestureDetectorInfo Function(Widget widget);

Then your custom function would just do:

if (widget is Avatar) {
  return RumGestureDetectorInfo('Avatar');
}

This probably would be better as a separate PR though.