Skip to content
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

Live debugging #999

Merged
merged 18 commits into from
Jun 11, 2024
Merged

Live debugging #999

merged 18 commits into from
Jun 11, 2024

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Jun 7, 2024

PR Description

This PR brings the live debugging feature to main.

Which issue(s) this PR fixes

Fixes #986

Notes to the Reviewer

This PR is made from three parts that have been reviewed already:

I added these two screenshots to the media folder:
ui_live_debugging_page
ui_component_detail_page_2

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated
  • Update the screenshots

wildum and others added 4 commits June 7, 2024 09:56
* create xray service to manage the debug streams

* add xray support to the prometheus.relabel component

* add xray endpoint to the api

* rename handler to manager

* defer cleanup func

* rework to add multi-streams support

* more tests

* additional multi delete test

* add todo comment for buffer size

* add error checks

* improve naming and readability, split interfaces and add a check to avoid expensive string computation

* move register and isregistered to a new debugRegistry interface
* add live debugging page in UI

* change button size

* move icon in front of text in buttons

* add warning if the number of incoming lines is greater than the buffer size
* add live debugging doc

* Update docs/sources/tasks/debug.md

Co-authored-by: Clayton Cornell <[email protected]>

* Update docs/sources/tasks/debug.md

Co-authored-by: Clayton Cornell <[email protected]>

* Update docs/sources/tasks/debug.md

Co-authored-by: Clayton Cornell <[email protected]>

* improve doc

* Update docs/sources/tasks/debug.md

Co-authored-by: Clayton Cornell <[email protected]>

---------

Co-authored-by: Clayton Cornell <[email protected]>
@wildum wildum requested a review from clayton-cornell as a code owner June 7, 2024 08:06
@wildum wildum requested a review from rfratto June 7, 2024 08:07
Copy link
Collaborator

@ptodev ptodev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I can't wait to try it!

@@ -48,6 +48,7 @@ Clicking a component in the graph navigates to the [Component detail page](#comp

### Component detail page

<!-- TODO: update this screenshot once the branch is ready to go to main because it contains an additional button now -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to just upload a second screenshot under a different name. That way if we rollback this change, the old markdown will fallback to using the old screenshot.

docs/sources/tasks/debug.md Outdated Show resolved Hide resolved
docs/sources/tasks/debug.md Show resolved Hide resolved
internal/service/livedebugging/livedebugging.go Outdated Show resolved Hide resolved
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I have a few small comments and suggestions for future improvements. I'm really looking forward to this expanding into more components :)

docs/sources/tasks/debug.md Show resolved Hide resolved
internal/component/prometheus/relabel/relabel.go Outdated Show resolved Hide resolved
internal/service/livedebugging/service.go Show resolved Hide resolved
internal/web/api/api.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@thampiotr
Copy link
Contributor

Good stuff! I have a few comments from testing it out, not necessarily blockers, but I'd like to raise them here.

  1. I may remembering incorrectly, but I thought this was meant to be explicitly enabled in a config file somehow? The reason is that an attacker with HTTP access to Alloy will be able to snoop the data which was previously not exposed or encrypted on the way out. We should give users an option to have this disabled IMO.

  2. There is a warning in dev console on Chrome, maybe unrelated to your changes, but sharing just in case:
    image

  3. I find that the button to enable Auto Scroll is tiny and hidden at the bottom, why not make it a normal button like others, next to Clear, Copy, etc?
    image

  4. When I filter the results and click Copy, I get all the data instead of just the filtered data. Is this intentional? I'd expect to only get what I have in the buffer.

  5. Nit: I am not sure about using => as a way to designate the mutation in prometheus.relabel. It may work for this component, but I think that other components may have more kinds of events (e.g. 'connected' or 'reloading targets' etc) and may end up with more logs-like data stream.... which now made me wonder if we could have implemented all of this using loggers, where you can peek into debug-level logs of any component on-demand. Would work with services too... kind a liking this idea now 😂

Overall, this is great and looking forward to having this on more components.

@wildum
Copy link
Contributor Author

wildum commented Jun 7, 2024

I find that the button to enable Auto Scroll is tiny and hidden at the bottom, why not make it a normal button like others, next to Clear, Copy, etc?

It's part of an imported react autoscroll component, I could not find a good way to bring it up and style it like the other buttons :(

@wildum
Copy link
Contributor Author

wildum commented Jun 7, 2024

Nit: I am not sure about using => as a way to designate the mutation in prometheus.relabel. It may work for this component, but I think that other components may have more kinds of events (e.g. 'connected' or 'reloading targets' etc) and may end up with more logs-like data stream.... which now made me wonder if we could have implemented all of this using loggers, where you can peek into debug-level logs of any component on-demand. Would work with services too... kind a liking this idea now

I think that these are two different things. The original idea of the x-ray feature is to see the data/the flow through the pipelines. Events data that you see in logs is not part of the plan. It does not replace logs but gives another tool that can be used alongside the others to troubleshoot Alloy.

@wildum
Copy link
Contributor Author

wildum commented Jun 7, 2024

I may remembering incorrectly, but I thought this was meant to be explicitly enabled in a config file somehow? The reason is that an attacker with HTTP access to Alloy will be able to snoop the data which was previously not exposed or encrypted on the way out. We should give users an option to have this disabled IMO.

Would it be ok if I create an issue for this and it comes in a follow up PR? I would create a block for the service and would add also other options to customize it (e.g. size of the buffer). I agree that it is important but not a blocker because having access to the Alloy's API but not its config file is not a very popular scenario I guess?

@wildum
Copy link
Contributor Author

wildum commented Jun 10, 2024

When I filter the results and click Copy, I get all the data instead of just the filtered data. Is this intentional? I'd expect to only get what I have in the buffer.

Fixed in 17aaf44

@wildum wildum requested a review from rfratto June 10, 2024 12:03
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 LGTM!

… The stability is also updated to experimental
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Jun 10, 2024
@wildum wildum requested a review from rfratto June 11, 2024 13:07
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % one comment (screenshots look good too). Thanks!

docs/sources/reference/config-blocks/livedebugging.md Outdated Show resolved Hide resolved
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions to remove the possessive "Alloy's"

docs/sources/reference/config-blocks/livedebugging.md Outdated Show resolved Hide resolved
docs/sources/reference/config-blocks/livedebugging.md Outdated Show resolved Hide resolved
docs/sources/reference/config-blocks/livedebugging.md Outdated Show resolved Hide resolved
@wildum
Copy link
Contributor Author

wildum commented Jun 11, 2024

I find that the button to enable Auto Scroll is tiny and hidden at the bottom, why not make it a normal button like others, next to Clear, Copy, etc?

FYI I created an issue for this: #1021

@wildum wildum merged commit 7aca045 into main Jun 11, 2024
18 checks passed
@wildum wildum deleted the xray branch June 11, 2024 17:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge live debugging to main
5 participants