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

321 packet logs #373

Merged
merged 4 commits into from
Nov 10, 2018
Merged

321 packet logs #373

merged 4 commits into from
Nov 10, 2018

Conversation

faeron
Copy link
Contributor

@faeron faeron commented Aug 29, 2018

📝 Description

🎯 Relevant issues

#321

💎 Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

🚦 How Has This Been Tested?

  • I started moleculer with npm dev script and defined transit.packetLogFilter for various packet types (heartbeat, discover, info) to verify that log messages do not appear.
  • Unit tests

🏁 Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have commented my code, particularly in hard-to-understand areas

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1481

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 92.003%

Totals Coverage Status
Change from base Build 1480: 0.007%
Covered Lines: 4632
Relevant Lines: 4813

💛 - Coveralls

@coveralls
Copy link

coveralls commented Aug 29, 2018

Pull Request Test Coverage Report for Build 1536

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 91.919%

Files with Coverage Reduction New Missed Lines %
src/transporters/tcp/tcp-reader.js 1 96.61%
src/service-broker.js 8 92.26%
Totals Coverage Status
Change from base Build 1528: 0.05%
Covered Lines: 4657
Relevant Lines: 4842

💛 - Coveralls

src/transit.js Outdated
@@ -36,7 +41,7 @@ class Transit {
this.logger = broker.getLogger("transit");
this.nodeID = broker.nodeID;
this.tx = transporter;
this.opts = opts;
this.opts = _.defaultsDeep(opts, defaultOptions);
Copy link
Member

Choose a reason for hiding this comment

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

Please don't duplicate default values. Use ServiceBroker.defaultOptions.transit instead.

@icebob
Copy link
Member

icebob commented Nov 4, 2018

@faeron Could you fix it?
I made benchmarks and there is no performance reduction.

@faeron
Copy link
Contributor Author

faeron commented Nov 5, 2018

Yup, I will fix it asap.

@faeron
Copy link
Contributor Author

faeron commented Nov 8, 2018

When trying to use ServiceBroker.defaultOptions.transitsome more tests are failing. It seems that there could be an issue with circular dependencies (Transit now requires ServiceBroker, which itself requires Transit). Any idea on how to resolve that?

@icebob
Copy link
Member

icebob commented Nov 9, 2018

Ohh, I see. In this case, drop the defaultOptions in the transit because it gets default options from broker where packetLogFilter is set to [] by default.
In brief, just use this option, no need to merge it with default values in contructor.

Thanks!

@faeron
Copy link
Contributor Author

faeron commented Nov 10, 2018

Should ready to get merged.

Copy link
Member

@icebob icebob left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@icebob icebob merged commit 2af043d into moleculerjs:master Nov 10, 2018
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

3 participants