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
exp: self instrumentation #981
Conversation
c21c3ca
to
532b611
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss a bit
@@ -320,14 +324,20 @@ func (sender *metricsIngestSender) sendBatches() { | |||
select { | |||
|
|||
case batch := <-sender.batchQueue: | |||
ctx := goContext.Background() | |||
ctx, txn := instrumentation.SelfInstrumentation.StartTransaction(ctx, "sender.sendBatches") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think to have one constants file with all transaction names listed there? Would be easier to modify namespace/subsystem/metric names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just thinking that we might want to have some dynamic (samples.sample, plugins.plugin ...) maybe we should wait until we have a list of what we want to report and then decide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 with the idea of having a file for transactions/spans names. The dynamic ones could be generated of a constant prefix + dynamic name.
The file could also be used to know what are we instrumenting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, it's just that I don't want to go too far this as this is just a POC. I'll create a doc to track this ideas/doubts and when it get's prioritised we can discuss these. wdyt?
t.nrSeg.End() | ||
} | ||
|
||
func NewAgentInstrumentationApm(license string, displayName string, apmEndpoint string, telemetryEndpoint string) (AgentInstrumentation, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the display name is not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 In fact I'm not even sure if they would need to be different per host 🤷♂️ Or just send it within the metrics. I'll make a test to see how it looks in the UI and then change this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to const name independent of host/display name ... In the UI we can do this later. Let's see with canaries if we like it or not and after we will be able to take a better decision. wdty?
4bdba79
to
ff1b215
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ff1b215
to
556e898
Compare
e9b9ba1
to
eba04ee
Compare
eba04ee
to
e2748d0
Compare
e2748d0
to
42dc359
Compare
42dc359
to
6538043
Compare
c5e4842
to
d76acb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.