-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Wildcard widget #25671
Add Wildcard widget #25671
Conversation
Preview links (active after the
|
ac2b405
to
2b6ecb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left you a few suggestions
|
||
{{< img src="/dashboards/widgets/wildcard/example_configuration_query.png" alt="Example widget configuration metric query for system.cpu.user grouped by env" style="width:100%;" >}} | ||
|
||
Notice the matching **query1** and **env** fields listed in the Vega-Lite specification and the Data Preview column. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't immediately clear to me what this JSON represents - and the highlighted values are env
and example
, so I wasn't sure where query1
came in. I think you could probably add a little more signposting around these examples to explain what you're showing the user. Is the point that the visualization starts out broken and then you fix it by reassigning the alias? If this is the case, it's useful to let the user know you're going to show them a broken example before the example appears.
Sidenote: I like the use of tables here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy paste error 😞 This should be query1
. I agree this could be broken down more. I added a new section specifically to map values between Datadog and Vega-Lite, hopefully that makes it clearer!
Co-authored-by: Heston Hoffman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
/merge |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
* Add Wildcard widget info * Fixes from code review * Apply suggestions from code review Co-authored-by: Heston Hoffman <[email protected]> * Add more context to map values --------- Co-authored-by: Heston Hoffman <[email protected]>
What does this PR do? What is the motivation?
Merge instructions
Do not merge Pending GA