Add Trace.recordRpcName to MysqlTracing class #250

wants to merge 5 commits into

7 participants


Without the Trace.recordRpcName call, all the Mysql spans shown up as unknown on zipkin traces.


This is useful, but you should also include the service name into the service field, e.g. service = "mysql." + serviceName.

You'll have to pass serviceName into MysqlTracing, like what finagle/http/Codec and finagle/thrift/ThriftClientFramedCodec do.

@sprsquish, what do you think? We are a bit inconsistent in how we set it in e.g. MemcachedTracingFilter.


@nshkrob Yes. I think it should be like this -

request match {
      case QueryRequest(sqlStatement) => {
        Trace.recordBinary("query", sqlStatement)
      case PrepareRequest(sqlStatement) => {
        Trace.recordBinary("prepare", sqlStatement)
      // TODO: save the prepared statement and put it in the executed request trace
      case ExecuteRequest(ps, flags, iterationCount) => {
        Trace.recordBinary("execute", "?")
      case CloseRequest(ps) => {
        Trace.recordBinary("close", "?")
      case _ => {
        Trace.recordRpcname("mysql", request.getClass.getName)
        Trace.record("mysql." + request.getClass.getName)

I didn't want to remove the original author's code, so as to not break their assumptions.

Also for 'Mysql' client, isn't the service name constant, unless the same Mysql client can be used to connect to different database/service.

Is there are scenario where the same client will be used for for other SQL services?


Looks good to me.
In most cases, there is just one mysql database in the system, so this will be good enough.

Twitter, Inc. member

Please make sure you've considered sharded cases, where someone will have hundreds/thousands mysql clients open, each with 3-5 hosts.


@roanta can you comment on that?


omit braces in multiline case statements

Twitter, Inc. member

After chatting with @sprsquish, I think we should thread through the service name in the filter and use it for tracing.

@sonnes Would mind updating the pr?


@stuhood I do not know about sharding on mysql very well. But if it's just the server host address & port. That case is already covered by the ServerAddr annotations. The server address shows up in the mysql span.

zipkin trace 453470a4559b1e16

Though, they show up as 'unknown' spans under mysql. I haven't spent time to find the source of these annotations in the code. Let me fix that as well.

@roanta will update the PR. will push the final changes along with the fix for the server addr. I should be able to spend time on this in a day or two.


@sonnes, I just wanted to follow up. Were you able to find the time to make the changes?


@mosesn Sorry about the delay. Got lost and forgot about this PR.

I have updated MySQL client as well to allow custom names for the client. This I believe should take care of cases where multiple clients are used in the same application.

I still couldn't find the source of the 'Unknown' spans (#250 (comment)).
What I have noticed is when am sampling 100% requests, these 'unknown' spans are added to the trace few minutes after the trace shows up on the UI. I think it's a case of repetitive tracing.

Twitter, Inc. member

It looks like this patch is slightly out of sync with our internal master. Let's temporarily put this on hold until next week when we will likely sync github with our internal repo.


No need to introduce a new parameter. Finagle clients should respect label as an identifier for clients.


@sonnes could you merge master? I think after you cover @roanta's change and merge master, we can merge it in.


@sonnes ping! Just want to make sure this doesn't fall through the cracks.


Sorry about the delay, again.
I have updated the filter to remove the deprecated tracing methods and removed the additional methods I added in the MySql client.
The latest code had changes that allow setting a label while initialising the client. The tracing filter now uses the label identifier from client

Client init looks like this:

lazy val sqlClient = Mysql

Can we keep this package private?

Yup. Missed that. Pushed a change (3e07fc5)

@sonnes sonnes Make MysqlTracing private

Could you format the patch into a single commit diffed against Finagle master, please? The patch generated by GitHub isn't playing nicely with git am since each patch is diffed against a different SHA.


@evnm let me do that.


@sonnes I did it for you, working on getting it merged in. Thanks for the contribution!


@sonnes OK, in internal review, a couple questions were raised.

A. Could you add a test for this? Maybe replacing the DefaultTracer with some sort of InMemoryTracer (like we do for StatsReceiver) and testing that the traces are properly recorded.

B. You shouldn't need to add the Trace.recordServiceName(clientName), in theory we should already be collecting that information. Have you found that's not the case?


OK, I'll close the PR for now, thanks for helping with this!

@mosesn mosesn closed this May 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment