-
Notifications
You must be signed in to change notification settings - Fork 793
Add ArangoDB Instrumentation #3829
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
base: main
Are you sure you want to change the base?
Conversation
9aa83e6
to
3b74aca
Compare
First time contributing here so looking for feedback. |
attributes = { | ||
db_attributes.DB_SYSTEM_NAME: "arangodb", | ||
db_attributes.DB_NAMESPACE: instance.db_name, | ||
db_attributes.DB_OPERATION_NAME: request.endpoint, | ||
db_attributes.DB_QUERY_TEXT: textwrap.dedent(query.strip("\n")), | ||
} |
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.
As per https://opentelemetry.io/docs/specs/semconv/database/database-spans/ server.address
/server.port
should also be included.
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.
Added server address and port info
else: | ||
query = str(request.data) | ||
|
||
attributes = { |
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.
Is there any case the table/collection could be extracted from the request and 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.
Potentially? It would likely only be a guess at best. ArangoDB queries use a "SQL-ish" language called AQL. AQL queries can operate on one or more collections at a time. I'm not sure how this is handled in other database implementations, but open to suggestions.
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.
So it would only be set if the query is known to only be operating on 1 table/collection. Looking at the http api there is api's for collections & documents would those be traced? If so you should be able to get collection from url.
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 those should be traced. However it looks like those APIs take the collection name as part of the URL path. So would likely need to try to parse it out
_api/collection/<collection-name>
or _api/document/<collection-name>
for key, value in bind_vars.items(): | ||
attributes[f"db.query.parameter.{key}"] = json.dumps(value) | ||
|
||
attributes["db.query.options"] = json.dumps(options) |
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.
This feels too generic for me, I would recommend identifying if there is any options which would be important and defining them as arrangodb.*
.
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 went through and explicitly defined which query options to pull into the attributes. There are more than what I have defined but these seemed to me to be the "most useful"
stats = extra.get("stats") | ||
for key, value in stats.items(): | ||
attributes["db.execution.stats." + key] = value |
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.
Would avoid this as it is too generic and I'd remove it. I would suggest populating the key stats into metrics.
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 just removed the stats part of this. I haven't really worked with metrics a whole lot so I will leave that as a future enhancement if needed and just focus on the traces.
warnings = extra.get("warnings") | ||
attributes["db.execution.warnings"] = json.dumps(warnings) |
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.
This feels more like data that would be on an event. I would leave off as no where else do we log warnings on the 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.
Moved these warnings to span events
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.
Looks better but I feel we need to be careful about how many attributes we are adding to ensure that they all bring value as each attribute has a cost associated with adding them.
if "count" in response.body: | ||
attributes["arangodb.response.count"] = response.body.get("count") |
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 count represent? Number of items?
if "hasMore" in response.body: | ||
attributes["arangodb.response.hasMore"] = response.body.get( | ||
"hasMore" | ||
) |
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 assume that has more is related to pagination? If so I don't see much benefit of it.
if "failOnWarning" in options: | ||
attributes[arangodb_attributes.FAIL_ON_WARNING] = options.get( | ||
"failOnWarning" | ||
) |
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.
This shouldn't be needed as the trace should convey this ie trace errored with xyz reason and trace has warning.
if "maxRuntime" in options: | ||
attributes[arangodb_attributes.MAX_RUNTIME] = options.get( | ||
"maxRuntime" | ||
) |
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.
Is this max execution time of query? If so would leave out as the error reason should convey when the limit has been hit.
if "fullCount" in options: | ||
attributes[arangodb_attributes.FULL_COUNT] = options.get( | ||
"fullCount" | ||
) |
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.
Leave this out as relates to stats
if "allowRetry" in options: | ||
attributes[arangodb_attributes.ALLOW_RETRY] = options.get( | ||
"allowRetry" | ||
) |
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 would leave this out as the usage is limited to batch reads & network failures.
if "cache" in options: | ||
attributes[arangodb_attributes.CACHE] = options.get("cache") |
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.
Rather than using true
/false
as the value, I would use skip
for false
& use
for true
.
|
||
ALLOW_RETRY = "arangodb.options.allowRetry" | ||
|
||
CACHE = "arangodb.options.cache" |
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.
Suggest naming this arangodb.query.cache
.
|
||
MAX_RUNTIME = "arangodb.options.maxRuntime" | ||
|
||
STREAM = "arangodb.options.stream" |
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.
Suggest naming this arangodb.query.stream
.
if "usePlanCache" in options: | ||
attributes[arangodb_attributes.USE_PLAN_CACHE] = options.get( | ||
"usePlanCache" | ||
) |
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 can't see this option in the js sdk, suggest removing it.
Description
Adds instrumentation for the
python-arango
libraryType of change
Please delete options that are not relevant.
How Has This Been Tested?
Originally implemented this instrumentation for a project at work. Unit tests verify ArangoDB client executions are intercepted and relevant request/response information is attached to the span.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.