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

hooks option of http plugin doesn't work for client mode #524

Closed
vitorbal opened this issue Apr 8, 2019 · 7 comments
Closed

hooks option of http plugin doesn't work for client mode #524

vitorbal opened this issue Apr 8, 2019 · 7 comments
Labels
community feature-request question Further information is requested

Comments

@vitorbal
Copy link

vitorbal commented Apr 8, 2019

I want to hook into the spans of the http plugin when in client mode. However, given the following code:

tracer.use('http', {
  hooks: {
    request: () => {
	  doSomething();
    }
  }
})

doSomething is never called when tracing http clients. I verified that it works for server mode, though.

Would it be possible to add support for hooks.request for client mode too?

@rochdev
Copy link
Member

rochdev commented Apr 8, 2019

Right now only web servers have hooks. We are trying to limit the use of hooks since usually when they are needed it means we are lacking something out of the box.

Maybe I can offer an alternative until we implement hooks for all integrations. What is your use case?

@rochdev rochdev added community feature-request question Further information is requested labels Apr 8, 2019
@vitorbal
Copy link
Author

vitorbal commented Apr 8, 2019

@rochdev thanks for the quick reply! My use case is the following:

The service used by the http plugin groups client calls by POST, GET, PATCH, etc resources:

However, I'd like to add some more granularity to these resources, so I can differentiate between e.g. calls to GET /api/v4* and GET /api/v3*.

I wanted to use the hook config to do something like the following:

tracer.use('http', {
  hooks: {
    request: (span, req) => {
	  span.setTag('resource.name', calculateResourceName(req))
    }
  }
})

Let me know if that makes sense and if you have any ideas how I could achieve this. Thanks in advance!

@rochdev
Copy link
Member

rochdev commented Apr 8, 2019

Definitely a legitimate use case. I'll try to see if we could add the feature for 0.11.0 since it's a common use case to manipulate the resource name for both HTTP servers and clients.

Since the public API doesn't currently allow accessing HTTP client spans, the only way to do it right now would be to access the span through the trace that is internally stored in every spans. If you want to go this route temporarily let me know and I'll show you how to do it.

@vitorbal
Copy link
Author

vitorbal commented Apr 8, 2019

Thanks @rochdev!

If you want to go this route temporarily let me know and I'll show you how to do it.

Yes, that would be great, if it's not asking too much :-)

@rochdev
Copy link
Member

rochdev commented Apr 8, 2019

So basically, the idea would be to get the HTTP span from the trace of the parent span currently in scope. If you have any blacklist of whitelist filter configured on the plugin, make sure to not do this for these URLs.

const req = http.request('http://localhost:8080/api/v4/users')

// Get the current active span so we can extract the trace from it.
const parent = tracer.scope().active()

// Safety net if there is no active span.
if (parent) {
  // Get spans that are not yet finished from most recent to least recent.
  const spans = [].concat(parent.context()._trace._started).reverse()

  // Find the last `http.request` span started. It will always be the last one
  // since it was started synchronously above. We still loop in case a child
  // within the http integration is started as well (i.e. tcp.connect).
  for (let span of spans) {
    if (span.context()._name === 'http.request') {
      span.setTag('resource.name', 'GET /api/v4/*')
      break
    }
  }
}

Of course this is not ideal, but it will at least unblock you until the HTTP client hook is implemented.

@vitorbal
Copy link
Author

vitorbal commented Apr 9, 2019

@rochdev thank you, this worked! I only had to change ._trace._started to ._trace.started. I'll keep an eye on #529 as well and try it out once it gets released! ❤️

@vitorbal
Copy link
Author

I think this issue can be closed now that #529 was merged. @rochdev feel free to re-open if you disagree. And thanks again for the quick resolution here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community feature-request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants