-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: Display Agent logs in Config Server UI #1462
Conversation
bbbbff6
to
02acfea
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.
Reviewed 6 of 9 files at r1.
Reviewable status: 6 of 9 files reviewed, 5 unresolved discussions (waiting on @MariusBrill and @Napfschnecke)
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 13 at r1 (raw file):
* Dialog that shows the given content, applying syntax highlighting if available, and offering a download option */ const DownloadDialog = ({ visible, onHide, error, loading, contentValue, contentType, contextName }) => {
I know we already had the typo in there beforehand, but could you fix it and name the file "DownloadDialogue"? when you make the changes suggested below?
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 14 at r1 (raw file):
*/ const DownloadDialog = ({ visible, onHide, error, loading, contentValue, contentType, contextName }) => { const fileConfig = ContentTypeMapper(contentType, contextName);
I may be nitpicking here but fileConfig is in my opinion not a good name for this variable. Since this component now not only contains config files, but also logs i think a name like dialogueContent would be more suitable.
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 17 at r1 (raw file):
const downloadFilename = contentType + (contextName ? '_' + contextName.replace(/[^a-z0-9-]/gi, '_').toLowerCase() : '') + fileConfig.fileExtension;
I am not quite sure here, but maybe it would be nicer to add the file extension to the props of this component? This would in my opinion make it easier to reuse it as you don't have to know the exact logic of this component to add another download window for a different file type.
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 44 at r1 (raw file):
<ProgressBar mode="indeterminate" /> ) : error ? ( <div>The agent configuration could not been loaded due to an unexpected error.</div>
I think we could add some more information to this error message. Something like "The agent configuration could not be loaded due to an unexpected error. Make sure you have the agent c
Suggestion:
The agent configuration could not be loaded due to an unexpected error. Ensure both agent commands and log preloading are enabled in your agent configuration.
components/inspectit-ocelot-configurationserver-ui/src/components/views/mappings/ContentTypeMapper.js
line 2 at r1 (raw file):
/** * mapping function that returns all values needed to setup the download of a file for a given content-type
Could you extend this documentation and add what contentTypes are valid inputs for this function?
02acfea
to
c1ac322
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.
Reviewable status: 5 of 9 files reviewed, 5 unresolved discussions (waiting on @MariusBrill and @Napfschnecke)
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 13 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I know we already had the typo in there beforehand, but could you fix it and name the file "DownloadDialogue"? when you make the changes suggested below?
I can of course change it.
Just to talk about it though: In a computing context, dialog is also very widely used and accepted (see e.g. https://developer.android.com/guide/topics/ui/dialogs)
Personally looks better to me without the weird "ue" at the end :D
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 14 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I may be nitpicking here but fileConfig is in my opinion not a good name for this variable. Since this component now not only contains config files, but also logs i think a name like dialogueContent would be more suitable.
fileConfig was not meant as referring to config files but the configuration of the file we are downloading. I agree that it can be mixed up with the other use of config in our context. dialogueContent would also not be fitting in my opinion, since the actual content of the dialogue and/or file is basically the only thing not covered in that variable.
Maybe something like dialogueSettings?
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 17 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I am not quite sure here, but maybe it would be nicer to add the file extension to the props of this component? This would in my opinion make it easier to reuse it as you don't have to know the exact logic of this component to add another download window for a different file type.
I was going back and forth between having more props and more component logic. I decided on this approach as it provided the cleanest interface for reuse of the component, while retaining flexibility by extending the ContentTypeMapper. This way we safe a lot of prop-passing in the statusview/statustable, since we only need the ContentType and everything else can be based on that.
If I put the file extension back as a normal prop I need to also add language for highlighting and the mimeType as prop again. Which will feel redundant when using the component...
I don't know. As I said, I changed it around a lot and this was the solution that seemed the cleanest to me :D
components/inspectit-ocelot-configurationserver-ui/src/components/views/dialogs/DownloadDialog.js
line 44 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
I think we could add some more information to this error message. Something like "The agent configuration could not be loaded due to an unexpected error. Make sure you have the agent c
Done.
c1ac322
to
d399d04
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.
Reviewable status: 5 of 9 files reviewed, 5 unresolved discussions (waiting on @MariusBrill)
components/inspectit-ocelot-configurationserver-ui/src/components/views/mappings/ContentTypeMapper.js
line 2 at r1 (raw file):
Previously, MariusBrill (Marius Brill) wrote…
Could you extend this documentation and add what contentTypes are valid inputs for this function?
Done.
d399d04
to
a826f64
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.
Reviewed 3 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Napfschnecke)
Closes #1454
Made some changes to the ShowConfigurationDialog (including renaming it) to make it more generically usable as a dialog for downloading logs or configs.
Changed existing code to reflect those changes and not change known behaviour.
Also included some tiny adjustments that were overlooked in previous PR's.
This change is