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

Added tracing support #3514

Merged
merged 4 commits into from
Sep 16, 2019

Conversation

ResuBaka
Copy link
Collaborator

@ResuBaka ResuBaka commented Sep 9, 2019

Related issues

closes #3506

Short description and why it's useful

This adds support to load a tracing library at the start of the app.

I have added an example for google cloud tracing.

You would use it like this.
src/trace/index.js

module.exports.default = () => {
require('./googleCloud')
}

Screenshots of visual changes before/after (if there are any)

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility and no breaking changes)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

With this we can now load a tracing lib on the start of the app.

Implement currently is google cloud tracing.
Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

That’s really clever idea! Thanks!

@pkarw pkarw requested review from patzick and filrak September 9, 2019 10:44
@pkarw
Copy link
Collaborator

pkarw commented Sep 9, 2019

@ResuBaka could you please check the failing CI?

@ResuBaka
Copy link
Collaborator Author

ResuBaka commented Sep 9, 2019

It looks like not a problem from my changes.

image
https://travis-ci.org/DivanteLtd/vue-storefront/builds/582552893?utm_source=github_status&utm_medium=notification

Should I still fix it or should some one else fix it?

Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

Sorry for tests, was my fault with merge, fixed:)

Although, don't you think it would be better to add this as a new module which can be included in the project, not by config, just by registering it? In develop we have rewritten modules system, @filrak could you take a look? :)

@pkarw
Copy link
Collaborator

pkarw commented Sep 9, 2019

I belive the problem is that modules can’t extend server.js @filrak this is pretty interesting case

Maybe we can just add a hook in core/server.js that can be bound from within the module just before first request is being handled?

@filrak maybe you could take care of this one by this chance:
#2494

@pkarw
Copy link
Collaborator

pkarw commented Sep 9, 2019

If not extending the modules for server app extensions then this way @ResuBaka proposed is the only way; the only thing is that’s gerrnarijg dependency from core to src but at this stage we do have these dependencies anyway so it’s fine with me

@pkarw
Copy link
Collaborator

pkarw commented Sep 9, 2019

Another idea is to add traces from the src/server - https://github.com/DivanteLtd/vue-storefront/blob/master/src/server/index.js - which won’t require us to modify core/server; these extensions are being registered only serverside

@ResuBaka
Copy link
Collaborator Author

ResuBaka commented Sep 9, 2019

I fist had that Idea too but all tracing stuff should happen as early as it gets. So they trace every thing the right way.

So with my implementation you can add all tracing frameworks which can be implemented this way be called directly before all other code gets loaded.

@patzick
Copy link
Collaborator

patzick commented Sep 10, 2019

@ResuBaka so let's wait for #3518 to be finished and then let's make first server module with your change, how does it sound for you? :)

@patzick patzick added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Sep 10, 2019
@ResuBaka
Copy link
Collaborator Author

I would use the server module but mine needs to be done before express is started/created so it would still be the best to have it like I have done.

Here you have some info. All of them are telling the user to start it as the first module.
https://docs.datadoghq.com/tracing/setup/nodejs/
image
https://www.elastic.co/guide/en/apm/agent/nodejs/current/express.html
image
https://www.npmjs.com/package/@google-cloud/trace-agent
image

@filrak
Copy link
Collaborator

filrak commented Sep 10, 2019

@ResuBaka interesting case, I will move extension point to the beginning

@pkarw
Copy link
Collaborator

pkarw commented Sep 10, 2019

@filrak so please just sync with @ResuBaka and let's have this ready to merge this week

@pkarw
Copy link
Collaborator

pkarw commented Sep 11, 2019

@ResuBaka @filrak - how about this one? Are we merging it in?

@ResuBaka
Copy link
Collaborator Author

I would say yes when we don't need typescript support for it or is that done in an other PR? @pkarw

@pkarw pkarw merged commit 09847dc into vuestorefront:develop Sep 16, 2019
@pkarw
Copy link
Collaborator

pkarw commented Sep 16, 2019

OK, I've merged it in

@filrak
Copy link
Collaborator

filrak commented Sep 18, 2019

@pkarw
image ;(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not ready for merge PR is holded. Needs some clarifications or things that need to be finished.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants