Are the listener methods always called in the same thread? If not, then this code isn't thread safe.

Yes, they are all called from the same thread https://www.mail-archive.com/[email protected]/msg48812.html

Yes, they are all called from the same thread https://www.mail-archive.com/[email protected]/msg48812.html

containsKey followed by get is redundant unless you store nulls into the map.
You can just do stageToJob.get(stageId). The only caveat is that you'll get back an Integer rather than int and then need to check against null.

Changed it to here

Integer jobId = stageToJob.get(stageId);
if (jobId == null) {
  return;
}

Seeing withStartTimestamp made me realize that we probably want to exclude these spans from tracking by profiling, since these spans aren't actually represent work within this JVM.

I'll make a note to think about that. It doesn't need to be a blocker for this initial prototype.

We actually don't to capture all values from config into a span.
That risks accidentally exposing sensitive information.

We need to build an inclusion list of config to include. We don't want to just include everything unconditionally.
It would also be fine to have the list be configurable.

Add the SparkConfAllowList to only capture a safe subset of the config. Is it what you meant by have the list be configurable ?

You shouldn't explicitly flush the tracer that's bad for performance.
Also, tracer.flush doesn't provide the semantics that you probably want. As currently implemented, it is really only good for testing.

Removed the flush(). The idea was that the JVM will be shut down by spark pretty quickly after the onApplicationEnd() event is received and this was to make sure that the spans are sent to datadog

Would you know if there is a way to ensure that ?

Is there a better message available on the applicationEnd object?

There is no error message in SparkListenerApplicationEnd, but we can provide the error message of the last job that failed

As with the config info, this needs to an inclusion list of know safe properties -- otherwise, there's a risk of leaking sensitive information.

Since you are dealing with two ints, you might as well just use a long if you want a composite key. Then you'll have no risk of accidental key collision.

It will also be more efficient to use shift operations / powers of 2 than multiplication.

Changed it to here

private long stageSpanKey(int stageId, int attemptId) {
  return ((long) stageId << 32) + attemptId;
}

Wouldn't it better to have two different allow lists? I imagine the parameters are different for app vs a job. Are they not?

You don't need the containsKey guard. You can just do a remove and test the result against null.
That's generally a better overall practice because it avoids rare race conditions when using maps synchronized across threads. It also happens to be a bit more efficient.

Removed all the remaining containsKey in this commit f69e40c

Since we're instrumenting a known type, org.apache.spark.SparkContext, you don't need isDeclaredBy here

Removed it in this commit 17a8ccf

this is another place we could use a small cache to reduce string creation

(note the .->_ replace can be made part of the caching function, so the lookup key is the original string)

this is another place we could use a small cache to reduce string creation

(note the .->_ replace can be made part of the caching function, so the lookup key is the original object)

this is another place we could use a small cache to reduce string creation