-
Notifications
You must be signed in to change notification settings - Fork 32
Conversation
Please update docs for this new plugin. |
|
You also should add a link for your plugin in the root readme doc, https://github.com/SkyAPM/go2sky-plugins#trace-plugins |
ok |
mongo/mongo.go
Outdated
span.SetComponent(ComponentMongo) | ||
span.SetSpanLayer(agentv3.SpanLayer_Database) | ||
span.Tag(go2sky.TagDBType, ComponentMongoDB) | ||
// span.Tag(go2sky.TagDBStatement, evt.Command.String()) |
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 happens for this statement tag?
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.
Some message executing the statement will be printed, but it will contain some indeterminate data.
eg.
{"create": "users","lsid": {"id": {"$binary":{"base64":"vtes1313Q1OnEMNazIsA1g==","subType":"04"}}},"$clusterTime": {"clusterTime": {"$timestamp":{"t":"1668405576","i":"1"}},"signature": {"hash": {"$binary":{"base64":"yFFWPXo66l1X7B91uKwSuZ3CMA8=","subType":"00"}},"keyId": {"$numberLong":"7122666363734196228"}}},"$db": “database"}
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.
Yes, tracing is for the internal messages. Could you add some configurations to disable or enable this in the tag?
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.
Read this.
If the statement(tag key = db.statement
existing, the backend would be able to sample the slow commands accordingly to thresholds settings.
665fd9f
to
0ab7af0
Compare
mongo/mongo.go
Outdated
ComponentMongoDB string = "MongoDB" | ||
|
||
// Peer peer. | ||
Peer string = "mongo:27017" |
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.
Why is peer a static?
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.
Under repair
mongo/mongo.go
Outdated
span, _, err := tracer.CreateLocalSpan(ctx, | ||
go2sky.WithSpanType(go2sky.SpanTypeEntry), |
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 does local span mean? Especially with span type entry?
Isn't this an exit span?
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.
Yes. It should be exit span.
span.SetPeer(Peer) | ||
span.SetComponent(ComponentMongo) | ||
span.SetSpanLayer(agentv3.SpanLayer_Database) | ||
span.Tag(go2sky.TagDBType, ComponentMongoDB) |
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.
You removed command tag?
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.
command tag is unpredictable. If necessary, can use Option to customize.
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.
Yes, I think an option is 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.
Should we wait for this new option?
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 mean? Do you want the implementation of Option to be part of the plugin?
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 don't have a preference. SkyWalking community just recommends the command tag.
So, I want to check with you, do you plan to add it?
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.
Ok, add command tag.
d1e9a46
to
72cbd83
Compare
Could you share what these tags look like? A screenshot or some logs. |
Thanks. So, this command is always in the tag, with no option to set. |
Yes. |
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, @arugal ptal
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
feat: add mongo plugin