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

Fix read ECONRESET error #145 #149

Merged
merged 4 commits into from Jan 26, 2022
Merged

Fix read ECONRESET error #145 #149

merged 4 commits into from Jan 26, 2022

Conversation

aaestrada
Copy link
Contributor

Hi @ovhemert! Thanks for this information! at issue #145 This will be so helpful for us! So i maked this pr with these changes?

Replace the event.error condition here

with

    if (!isCustomTagsLoggingIgnored(event, ignoredEventTags.log)) { // first check on ignoring tags
      if (event.error) {
        logger.error({ err: event.error })
      } else {
        logEvent(logger, event)
      }
    }

so after this change should be able to ignore this events tags as you are showing here... right?

  await server.register({
    plugin: require('hapi-pino'),
    options: {
      prettyPrint: true,
      ignoredEventTags: { log: ['client'], request: '*' }
    }
  })

@coveralls
Copy link

coveralls commented Nov 24, 2021

Coverage Status

Coverage increased (+0.0004%) to 98.99% when pulling 207bd4b on aaestrada:master into f1af9be on pinojs:master.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Can you please add a unit test?

@mcollina
Copy link
Collaborator

There is a conflict, could you rebase?

@mcollina
Copy link
Collaborator

tests are failing, could you take a look?

@mcollina mcollina merged commit 97a77cb into hapijs:master Jan 26, 2022
@jonathansamines
Copy link
Contributor

This pull request seem to have reverted some changes introduced at #151 (specifically for server logs). Is that expected @mcollina?

@mcollina
Copy link
Collaborator

Not really and sorry for the breakage. Would you like to help maintaining this module? I don't have time and/or context as it has been long before I used Hapi in production.

@mcollina
Copy link
Collaborator

Feel free to bring those changes back it if you want to.

@aaestrada
Copy link
Contributor Author

aaestrada commented Jan 31, 2022

Hi @jonathansamines ! With the config in this pr description. Our server give us a very, very good performance results.
Otherwise with the pass config there was a ECONRESET as @ovhemert describe here.

Could we share more details at this pr in order to find the correct config needed for production?

See here an example of our config..

https://github.com/aaestrada/pino-tranport/blob/master/pino-file/index.js

We ran some sintetic tests with blazemeter in production we have:

Screen Shot 2022-01-31 at 8 43 58

I'll happy to collaborate with you to find the best solution here...

@mcollina
Copy link
Collaborator

I don't understand @aaestrada. I'll be happy to review a PR.

@jonathansamines
Copy link
Contributor

Feel free to bring those changes back it if you want to.

Hey @mcollina. I just created #153 to restore support for tags in server.log() calls.

Would you like to help maintaining this module? I don't have time and/or context as it has been long before I used Hapi in production.

Sure, what kind of help are you looking for?

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.

None yet

4 participants