Allow overriding RESTDataSource.trace

[christianrondeau]

I'm currently adding Application Insights' dependency tracking into an Apollo server, and I'd like to track the time it takes to call dependencies from within Apollo. Good news, there's a trace method that does exactly what I need! Bad news, it's private and only logs to console...

I'd like to simply be able to receive the trace context (url, duration, method) and log that information myself.

I'm happy to provide a PR if that makes sense.

1. New boolean property RESTDataSource.traceEnabled (by default, process && process.env && process.env.NODE_ENV === 'development')
2. Change RESTDataSource.trace to receive individual properties options.method || 'GET' and url
3. New protected function RESTDataSource.log, which by default logs to the console like it does now, but that can be overriden.

[christianrondeau]

So for your information, I started a branch, which is pretty much "ready" (there were no tests for the trace method): https://github.com/apollographql/apollo-server/compare/master...christianrondeau:gh-2009?expand=1

Quick notes:

• I added a success property, which can be very useful when filtering
• I'd really like to add a resultCode property too. It's in fact simple when there's an error (using err.extensions.response.status) but harder when it succeeds :) I'd need to do a little bit more refactoring to non-private methods (e.g. didReceiveResponse could return a Promise<[TResult,Response]>, which would be reduced to a simple Promise<TResult> in the trace method.

I'd like to head feedback from a maintainer before further modifying the code.

[k-komarov]

So for your information, I started a branch, which is pretty much "ready" (there were no tests for the trace method): https://github.com/apollographql/apollo-server/compare/master...christianrondeau:gh-2009?expand=1

Quick notes:

• I added a success property, which can be very useful when filtering
• I'd really like to add a resultCode property too. It's in fact simple when there's an error (using err.extensions.response.status) but harder when it succeeds :) I'd need to do a little bit more refactoring to non-private methods (e.g. didReceiveResponse could return a Promise<[TResult,Response]>, which would be reduced to a simple Promise<TResult> in the trace method.

I'd like to head feedback from a maintainer before further modifying the code.

Any estimations when it will be merged?

[marcolanaro]

Is there any news on this? :)

[christianrondeau]

For your information, I stopped using apollo's rest data source completely since, the amount of things I had to workaround was larger than the gain for me.

[halfzebra]

@christianrondeau thanks for the feedback, are there any other viable alternatives to RESTDataSource?

I want to gather metrics on how my data sources are performing when they actually send requests.
Overriding trace seems like a perfect interface for this.

@abernix Are there any plans on making trace method protected instead of private?

[lmcorbo]

Is there any news on this?

[borekb]

@halfzebra

Are there any plans on making trace method protected instead of private?

There even was a PR trying to convert trace into a protected method, #2182, but it was closed by the author.

---

In our case, we just want to get rid of the logs like these:

GET https://my-api-endpoint/... (123ms)

We have our own logging and it's unpleasant that console.log is hardcoded for NODE_ENV=development:

apollo-server/packages/apollo-datasource-rest/src/RESTDataSource.ts

Lines 285 to 293 in bbae424

	if (process && process.env && process.env.NODE_ENV === 'development') {
	// We're not using console.time because that isn't supported on Cloudflare
	const startTime = Date.now();
	try {
	return await fn();
	} finally {
	const duration = Date.now() - startTime;
	console.log(`${label} (${duration}ms)`);
	}

[itmayziii]

I know this issue has been opened for awhile, but I would also like to point out that the implementation for trace is using console.log instead of logger that Apollo Server takes in the constructor. It would be beneficial for all logs in the Apollo ecosystem to be going through this provided logger (even if the provided logger just delegates back to console.) This decision should be made by the library user and not forced by the library author.

[wabrit]

In the absence of a proper fix (which I agree would be highly desirable) you can "subclass" RESTDataSource and provide your own override of the trace(label,fn) implementation (even though its marked private) as the body of that method isn't particularly complex; it's not ideal, or pretty, but it does appear to work.

[rval]

Major +1 on this issue. It may not seem like a big deal, but having an unnecessary console.log on every run of our test suite really litters the console with a lot of noise, and makes finding actual test failures that much more difficult.

Simply making this method protected instead of private seems like an easy win, and I see there are PRs to do just that (first, second).

[bitworking]

I've just overriden the method with @ts-ignore. But what would be the best way to also trace the REST calls response status? Wouldn't it be better to trace in "didReceiveResponse" or something?

[wabrit]

I think you can put some tracing in didReceiveResponse and didEncounterError