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

others(operate-importer): add more logs during the import of process definitions #3512

Conversation

mathias-vandaele
Copy link
Contributor

Adding logs during the importation of process definitions

Description

This PR add logs tracing what is happening during the importation of the process definition from Operate.

Related issues

closes #3190

Checklist

  • PR has a milestone or the no milestone label.

@mathias-vandaele mathias-vandaele requested a review from a team as a code owner October 18, 2024 13:39
@mathias-vandaele mathias-vandaele linked an issue Oct 18, 2024 that may be closed by this pull request
@sbuettner
Copy link
Contributor

@chillleader Can you please take a look whether these log statements would help? It wasnt clear to me based on the ticket whether trace/debug will help as they are usually not shown using the default log levels.

@mathias-vandaele
Copy link
Contributor Author

@chillleader Can you please take a look whether these log statements would help? It wasnt clear to me based on the ticket whether trace/debug will help as they are usually not shown using the default log levels.

From my understanding, when a lot of process are present, this can take lots of time, as we use the operate client to retrieve all of them page by page, then sorting, then filtering out etc..
Those logs add informations about the step we are on, depending on the severity of it, It is in trace, debug or info

@sbuettner
Copy link
Contributor

sbuettner commented Oct 24, 2024

@mathias-vandaele If we want to get more information about "where we are in the process" we would benefit from some kind of "progress" or number of pages we already scanned.

chillleader
chillleader previously approved these changes Oct 24, 2024
Copy link
Member

@chillleader chillleader left a comment

Choose a reason for hiding this comment

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

@chillleader Can you please take a look whether these log statements would help? It wasnt clear to me based on the ticket whether trace/debug will help as they are usually not shown using the default log levels.

I think these changes will be helpful - although they are not shown by default due to logging level (only info and above are shown), it's quite easy to increase the logging level if we want to gain access to this info. I was originally looking for some progress indication, as well as a way to know which process IDs are being processed - I think this PR solves this well.

The only suggestion from my side: I'd rather keep all detailed log entries under the DEBUG or INFO level (depending on whether we want to show them by default) and not introduce the TRACE level to keep things a little less complicated

@mathias-vandaele
Copy link
Contributor Author

@chillleader Can you please take a look whether these log statements would help? It wasnt clear to me based on the ticket whether trace/debug will help as they are usually not shown using the default log levels.

I think these changes will be helpful - although they are not shown by default due to logging level (only info and above are shown), it's quite easy to increase the logging level if we want to gain access to this info. I was originally looking for some progress indication, as well as a way to know which process IDs are being processed - I think this PR solves this well.

The only suggestion from my side: I'd rather keep all detailed log entries under the DEBUG or INFO level (depending on whether we want to show them by default) and not introduce the TRACE level to keep things a little less complicated

I will change the trace into debug 👍

@mathias-vandaele mathias-vandaele added this to the 8.7.0-alpha1 milestone Oct 25, 2024
Copy link
Member

@chillleader chillleader left a comment

Choose a reason for hiding this comment

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

Thank you!

@mathias-vandaele mathias-vandaele added this pull request to the merge queue Oct 25, 2024
Merged via the queue into main with commit 74bd0b9 Oct 25, 2024
10 of 11 checks passed
@mathias-vandaele mathias-vandaele deleted the 3190-add-more-logging-during-process-definition-importing branch October 25, 2024 10:05
mathias-vandaele added a commit that referenced this pull request Oct 28, 2024
…definitions (#3512)

* others(operate-importer): add more logs during the import of process definitions

* others(logs): format files

* others(logs): format files 2 and remove trace logs
mathias-vandaele added a commit that referenced this pull request Oct 28, 2024
…definitions (#3512)

* others(operate-importer): add more logs during the import of process definitions

* others(logs): format files

* others(logs): format files 2 and remove trace logs
mathias-vandaele added a commit that referenced this pull request Nov 8, 2024
…definitions (#3512)

* others(operate-importer): add more logs during the import of process definitions

* others(logs): format files

* others(logs): format files 2 and remove trace logs
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.

Add more logging during process definition importing
3 participants