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

Closes #1495 - improve the behavior when agent commands fail (or are not available) due to missing configuration - frontend #1528

Merged
merged 45 commits into from
Nov 14, 2022

Conversation

danipaniii
Copy link
Contributor

@danipaniii danipaniii commented Oct 14, 2022

Closes #1495
Implementing the DynamicallyServiceObserver to register all the available services (e.g. Prometheus, Jaeger, LogPreloader) and their current state enabled/disabled.
In the UI when clicking the agents name (on the status page) a dialog window opens with a data table, listing all the services and their current state enabled/disabled.
Currently the states are being shown as disabled buttons with the labes (enabled/disabled) to show the user the current state. In the future there is the possibilty to implement a feature to activate those buttons and let the user active/deactivate a service by a simple button click.


This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Merging #1528 (0c51ee7) into master (4ae7a26) will decrease coverage by 0.72%.
The diff coverage is 13.33%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1528      +/-   ##
============================================
- Coverage     78.64%   77.92%   -0.72%     
- Complexity     2271     2285      +14     
============================================
  Files           232      235       +3     
  Lines          7438     7563     +125     
  Branches        887      902      +15     
============================================
+ Hits           5849     5893      +44     
- Misses         1219     1302      +83     
+ Partials        370      368       -2     
Impacted Files Coverage Δ
.../src/main/java/com/mindprod/jarcheck/JarCheck.java 0.00% <0.00%> (ø)
...service/DynamicallyActivatableServiceObserver.java 81.82% <81.82%> (ø)
.../propertysources/http/HttpPropertySourceState.java 77.86% <100.00%> (+0.17%) ⬆️
...ot/core/service/DynamicallyActivatableService.java 83.72% <100.00%> (+0.79%) ⬆️
...elot/config/model/exporters/CompressionMethod.java 100.00% <0.00%> (ø)
.../ocelot/core/metrics/system/GCMetricsRecorder.java 72.95% <0.00%> (+0.82%) ⬆️
...ot/core/instrumentation/AsyncClassTransformer.java 85.45% <0.00%> (+0.91%) ⬆️
...ocelot/core/exporter/OtlpTraceExporterService.java 66.67% <0.00%> (+1.80%) ⬆️
...t/ocelot/config/validation/PropertyPathHelper.java 78.89% <0.00%> (+2.22%) ⬆️
...elot/core/exporter/OtlpMetricsExporterService.java 69.49% <0.00%> (+3.64%) ⬆️
... and 1 more

@danipaniii danipaniii changed the title Feature 1495 Feature 1495: DASO and UI Oct 17, 2022
@danipaniii danipaniii changed the title Feature 1495: DASO and UI Closes(#1495) - improve the behavior when agent commands fail (or are not available) due to missing configuration Oct 17, 2022
@heiko-holz heiko-holz changed the title Closes(#1495) - improve the behavior when agent commands fail (or are not available) due to missing configuration Closes #1495 - improve the behavior when agent commands fail (or are not available) due to missing configuration Oct 17, 2022
@heiko-holz heiko-holz changed the title Closes #1495 - improve the behavior when agent commands fail (or are not available) due to missing configuration Closes #1495 - improve the behavior when agent commands fail (or are not available) due to missing configuration - frontend Oct 17, 2022
Copy link
Contributor Author

@danipaniii danipaniii left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 14 files reviewed, 3 unresolved discussions (waiting on @heiko-holz)


components/inspectit-ocelot-configurationserver-ui/.yarn/install-state.gz line at r4 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

same as with the other pull requests, this file can be deleted and the .gitignore updated

done


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js line 180 at r5 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Currently, I am missing a tooltip for the badge that tells the user that it opens the Service State Dialogue.
Additionally, the mouse should then be a pointer (as for the buttons).

Or we add a new button to invoke the service state dialogue.

I added a new button with a tooltip.


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/dialogs/ServiceStateDialog.js line 29 at r5 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

the list seems to be unordered (or the order is defined by the config-server).

I think we can add a more intuitive order.

Maybe add "categories" (e.g., exporter) and indent the services into their categories.
For the categories, we could just take the hierarchically next property.

For now I added a functionality to order the columns by the field. So it's possible to order the name by alphabetical order and the state by enabled/disabled, also in ascending or descending order.
Maybe we can talk about the sorting by categories, because I think this will need a little bit more thinking to implement.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danipaniii)


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js line 180 at r5 (raw file):

Previously, danipaniii wrote…

I added a new button with a tooltip.

I really like it :)


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js line 296 at r7 (raw file):

    this.setServiceStateDialogShown(true);
    this.setState({
      settingStates: settingStates,

Should these settingStates be named serviceStates?


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/dialogs/ServiceStateDialog.js line 29 at r5 (raw file):

Previously, danipaniii wrote…

For now I added a functionality to order the columns by the field. So it's possible to order the name by alphabetical order and the state by enabled/disabled, also in ascending or descending order.
Maybe we can talk about the sorting by categories, because I think this will need a little bit more thinking to implement.

The sorting is very nice :)

Let's discuss other functionality of this view in the next weekly or a next meeting and make it another issue.

One more thing that I realized is that the buttons of the modal are named back and accept. Currently, there should only be one button with something like okay or close. Or what should the buttons do?

heiko-holz
heiko-holz previously approved these changes Nov 7, 2022
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r8, 5 of 7 files at r9, 1 of 1 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danipaniii)


components/inspectit-ocelot-configurationserver-ui/.gitignore line 3 at r10 (raw file):

node_modules
.next
.yarn/*

Let's discuss this in today's sync.

See https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danipaniii)


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js line 296 at r7 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Should these settingStates be named serviceStates?

this has been fixed

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danipaniii)

heiko-holz
heiko-holz previously approved these changes Nov 10, 2022
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @danipaniii)


components/inspectit-ocelot-configurationserver-ui/.gitignore line 3 at r10 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Let's discuss this in today's sync.

See https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored

updated the .gitignore accordingly

# Conflicts:
#	components/inspectit-ocelot-configurationserver-ui/.gitignore
#	components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js
danipaniii added a commit to danipaniii/inspectit-ocelot that referenced this pull request Nov 11, 2022
…spectIT#1528)

* Update Readme to help developers handle prettier errors in the Config UI tests

* Minor improvements in README for prettier trouble shooting

Co-authored-by: Priebe Daniel <dpr@novatec-gmbh.de>
Co-authored-by: Heiko Holz <heiko.holz@novatec-gmbh.de>
@danipaniii
Copy link
Contributor Author

Everything seems to be working as expected.
@heiko-holz in our last conversation we talked about the badge that is being shown in the status view and the badge overlapping the buttons.
The badge doesn't overlap with the buttons anymore, but slightly overlaps the agents service name, if the name is very long (See screenshot)
badge_longname
Since we talked about improving the styling in that part, I was wondering if we should open a new Issue to resolve this or if we should at least solve the overlapping with the badge and the name at the moment.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 7 of 24 files reviewed, all discussions resolved (waiting on @danipaniii)

@heiko-holz
Copy link
Contributor

Hey @danipaniii, good catch!
I just fixed this by reducing the max-width for the service name.

Anyhow, we can open an issue after this PR to improve the code and style of the Agent Status Table.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r14, 14 of 15 files at r16, 1 of 1 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @danipaniii)

@heiko-holz heiko-holz merged commit 5637a4a into inspectIT:master Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] - improve the behavior when agent commands fail (or are not available) due to missing configuration
2 participants