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

Dev UI - add basic monitoring of CDI components #15040

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Feb 12, 2021

  • by default, fired events and business method invocations are monitored
  • quarkus.arc.dev-mode-monitoring=false disables the feature

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/vertx and removed area/dev-ui labels Feb 12, 2021
gsmet
gsmet previously requested changes Feb 12, 2021
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added a small comment/question.

@gsmet gsmet dismissed their stale review February 12, 2021 15:30

Brain freeze.

@mkouba mkouba marked this pull request as draft February 14, 2021 18:28
@mkouba mkouba force-pushed the dev-ui-arc-fired-events branch from acf51a0 to 4913445 Compare February 16, 2021 09:17
@mkouba mkouba changed the title Dev UI - add CDI fired events view Dev UI - add basic monitoring of CDI components Feb 16, 2021
@mkouba mkouba marked this pull request as ready for review February 16, 2021 09:18
@geoand geoand requested a review from manovotn February 17, 2021 09:30
@mkouba mkouba force-pushed the dev-ui-arc-fired-events branch from 4913445 to 3570b99 Compare February 17, 2021 13:17
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

I played with this on some quickstarts and I think it is a great tool! Also, using CDI to monitor CDI is brilliant ;-)
Left just two minor comments, otherwise I am simply +1 for this.

@mkouba mkouba force-pushed the dev-ui-arc-fired-events branch from 3570b99 to 2a33fb6 Compare February 17, 2021 14:37
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I played a bit with it with my Quarkus Bot Java app.

A couple of things:

  • fired events are numbered from 1. but they are sorted in anti-chronological order. So every time you fire a new event, the events are renumbered, which is weird.
  • for invocation trees, I think you should had % based width for the table. Otherwise, every time I click on the + icon in the tree, the width changes, which is a bit annoying.
  • still invocation trees, I find it weird you don't sum up the invocation times of the tree somehow. Maybe you should have two values. See screenshot.
    Screenshot from 2021-02-18 13-54-39

@mkouba
Copy link
Contributor Author

mkouba commented Feb 22, 2021

fired events are numbered from 1. but they are sorted in anti-chronological order. So every time you fire a new event, the events are renumbered, which is weird.

Yes, the latest events/invocations are listed first and the number is just an index, a position in the table. We could just skip this column but I always find it useful when displaying a table...

for invocation trees, I think you should had % based width for the table. Otherwise, every time I click on the + icon in the tree, the width changes, which is a bit annoying.

Ok, I can try do something with this but I'm no CSS master ;-). (it looks ok in FF BTW)

still invocation trees, I find it weird you don't sum up the invocation times of the tree somehow. Maybe you should have two values.

Hm, that could be a bug with CP or something like that. The duration of the root must always be greater than the sum of the children.

@gsmet
Copy link
Member

gsmet commented Feb 23, 2021

Yes, the latest events/invocations are listed first and the number is just an index, a position in the table. We could just skip this column but I always find it useful when displaying a table...

It's useful if it's stable. If it changes every time we get an event, I think it's not such a good idea. Can't you have 1. for the oldest event instead? This way, you would have a number and something that's stable.

If not, I would drop it.

Ok, I can try do something with this but I'm no CSS master ;-). (it looks ok in FF BTW)

I think just adding style="width:xx%" to the ths should be enough. If not I can have a look.

Hm, that could be a bug with CP or something like that. The duration of the root must always be greater than the sum of the children.

Well, you have seen my screenshot :). Who could help on this?

@mkouba mkouba force-pushed the dev-ui-arc-fired-events branch 2 times, most recently from 525855a to aa05c5f Compare February 23, 2021 13:28
@mkouba
Copy link
Contributor Author

mkouba commented Feb 23, 2021

@gsmet all your commets should be addressed now ;-)

- by default, fired events and business method invocations are monitored
- quarkus.arc.dev-mode-monitoring=false disables the feature
@mkouba mkouba force-pushed the dev-ui-arc-fired-events branch from aa05c5f to 8cd0426 Compare February 23, 2021 13:30
@mkouba mkouba requested a review from gsmet February 23, 2021 13:32
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

I am just putting my green stamp here as all my previous comments were addressed.
But we should still wait until Guillaume gives his approval.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Woot, thanks! That's very useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/vertx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants