-
Notifications
You must be signed in to change notification settings - Fork 1
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
Gsren 315 thread safe logging #50
Conversation
…tions are not caught in try: catch:
backend/maelstro/core/operations.py
Outdated
url=response.url, | ||
data_type=self.context, | ||
) | ||
self.responses[self.id].append(api_record) |
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.
As far as I understand, it is not really useful to keep track of logs not related to self.id.
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.
yes, you are right the id is no longer useful since an independent LogHandler instance is attached to each request state
will fix
except AttributeError: | ||
self.responses.append(None) | ||
self.responses[self.id].append( | ||
InfoRecord( |
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.
In which case this happens (AttributeError) ?
Is it really an info or an error ?
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.
this part is reached of extra=response is not added to the log (if there is a wrong version of the lib.
This cas will not be reached by design, but since the app cannot be sure if record contains a response or not, it will have to try. It is a root log message, so most likely the lib has something to say and it would be a pity to drop the message.
I would propably keep this code and add a comment
backend/maelstro/core/operations.py
Outdated
|
||
def get_json_responses(self) -> list[dict[str, Any]]: | ||
return [self.json_response(r) for r in self.responses if r is not None] | ||
def pop_properties(self) -> dict[str, Any]: |
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.
This is not a pop but a get.
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.
yes, i was a pop and became a get
) | ||
server=gs_info["url"], | ||
user=gs_info["auth"] and gs_info["auth"].login, | ||
err="Invalid credentials", | ||
) from err | ||
gs_logger.info( | ||
"Session opened on %s at %s", |
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.
resp may be not associated with a value in following lines if status code is not 401.
missing a raise err
?
For example:
docker compose stop geoserver
Then run copy.
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.
OK, reraise original error
|
||
const styleLogs = computed(() => logs.value.filter((l) => l.data_type == 'Style')) | ||
|
||
const metaSuccessful = computed(() => metaLogs.value.some((l) => l.status == "OK")) |
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.
No feedback here in case of error.
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.
OK added red cross
Refactoring to enable: