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

Adds an option to override default error log formatting #1570

Merged
merged 7 commits into from
Sep 1, 2020

Conversation

greenzelenetskyi
Copy link
Contributor

Move action error logline formatting logic from actionProcessor to error serializers to make it possible to override default action error logging, as previously discussed in #1561

@greenzelenetskyi greenzelenetskyi changed the title Action error logging Adds an option to override default error log formatting Aug 26, 2020
@greenzelenetskyi greenzelenetskyi marked this pull request as ready for review August 26, 2020 14:49
@greenzelenetskyi
Copy link
Contributor Author

Hi, I'm not sure, what could've caused the failing test after the merge. I'd appreciate any help.

@evantahler
Copy link
Member

Looks like it was just a timeout in the Browser integration test:

    Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 5000 ms timeout specified by jest.setTimeout.

I'll run them again! In the future, if you want to run it again yourself, just push another empty commit git commit --allow-empty -m "Trigger Build"

Copy link
Member

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

This looks like a great addition!

I'd suggest adding a little more safety around your call to config.errors.serializers.actionProcessor. As written, this would be a breaking change to Actionhero, as it won't work without adding the new method to config/errors. Is there a (possibly simplified) default behavior you can add if config.errors.serializers.actionProcessor doesn't exist?

@witem
Copy link
Contributor

witem commented Aug 26, 2020

This looks like a great addition!

I'd suggest adding a little more safety around your call to config.errors.serializers.actionProcessor. As written, this would be a breaking change to Actionhero, as it won't work without adding the new method to config/errors. Is there a (possibly simplified) default behavior you can add if config.errors.serializers.actionProcessor doesn't exist?

But it also breaks old log format. Maybe need to move default serializer to classes/actionProcessor.ts. And add possibility to redefine it over config.errors.serializers.actionProcessor if it exists.

@greenzelenetskyi
Copy link
Contributor Author

Great points. Thanks. I've moved the default logic back to actionProcessor and you use it as a fallback to the serializer. What do you think?

src/config/errors.ts Show resolved Hide resolved
@evantahler
Copy link
Member

Awesome work!

@evantahler evantahler merged commit 827dbb4 into actionhero:master Sep 1, 2020
@evantahler evantahler added the enhancement New feature or request label Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants