StartSpanOptions ignored options parameter

[ChristophPech]

No description provided.

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[axw]

@ChristophPech can you please explain what problem this is addressing? Preferably by adding a test case which fails without / passes with the change.

We already do this in Transaction.StartSpanOptions, which is eventually called by apm.StartSpan:

apm-agent-go/span.go

Lines 68 to 73 in 797c31c

	}
	transactionID := tx.traceContext.Span
	if opts.Parent == (TraceContext{}) {
	opts.Parent = tx.traceContext
	}
	// Calculate the span time relative to the transaction timestamp so

[ChristophPech]

I am using go-pg and wrote a callback to trace postgres calls. I have to set the StartTime retroactively because there is only a "finished" callback so I used apm.StartSpanOptions(...)

Yes, in hindsight I could have uses Transaction.StartSpanOptions(), but the function apm.StartSpanOptions() completely ignores the opts parameter, which is a problem.

[axw]

I am using go-pg and wrote a callback to trace postgres calls. I have to set the StartTime retroactively because there is only a "finished" callback so I used apm.StartSpanOptions(...)

Understood.

Yes, in hindsight I could have uses Transaction.StartSpanOptions(), but the function apm.StartSpanOptions() completely ignores the opts parameter, which is a problem.

Sorry, I misread the issue initially, and I was looking at apm.StartSpan and not apm.StartSpanOptions. I understand now.

There's a couple of issues with the patch - I'll respond with review comments.

[axw]

If you are able to add a small test that would be great, otherwise I can do it next week.

[codecov-io]

Codecov Report

Merging #326 into master will decrease coverage by 3.67%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #326      +/-   ##
=========================================
- Coverage   84.78%   81.1%   -3.68%     
=========================================
  Files          98      98              
  Lines        5979    5891      -88     
=========================================
- Hits         5069    4778     -291     
  Misses        841     841              
- Partials       69     272     +203

Impacted Files	Coverage Δ
apmtest/withtransaction.go	85.71% <100%> (-4.29%)	⬇️
span.go	86.56% <100%> (-6.61%)	⬇️
gocontext.go	100% <100%> (ø)	⬆️
internal/sqlscanner/token.go	60% <0%> (-20%)	⬇️
internal/apmhostutil/docker_linux.go	62.5% <0%> (-12.5%)	⬇️
utils_linux.go	77.77% <0%> (-11.12%)	⬇️
stacktrace/context.go	70% <0%> (-10%)	⬇️
model/marshal_fastjson.go	72.97% <0%> (-9.25%)	⬇️
sampler.go	81.81% <0%> (-9.1%)	⬇️
module/apmsql/utils.go	18.18% <0%> (-9.1%)	⬇️
... and 34 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e26b76...b81d63f. Read the comment docs.

[axw]

@ChristophPech did you have a chance to test this out yet?

Let me know if you'd like me to take it over. There's no rush though, I'm happy to wait if you're keen to finish it off.

[ChristophPech]

+axw Yes, I am running your changes now on my staging server for several days and it works as expexted. I used go mod to test it and have now merged it.

[axw]

Thank you very much @ChristophPech! You're the first contributor outside of @elastic 🥇