Comments (27)
+1 to the idea that if net/http
lives here, then so should database/sql
from opentelemetry-go-contrib.
I'm not 100% sure I understand why this wouldn't be hosted here. I get offloading instrumentation for public packages to their repos, but I don't see how that applies to stdlib packages. This database/sql
package is specifically called out there, with this as the required import path. go.opentelemetry.io/contrib/instrumentation/database/sql/otelsql
Can anyone create a standalone repo to claim that import path? It seems to me that any stdlib instrumentation should live here in the contrib repo.
from opentelemetry-go-contrib.
@MrAlias any feedback on this?
from opentelemetry-go-contrib.
From the SIG meeting today:
@Aneurysm9 mentioned that possibly Honeycomb might be able to contribute their DB instrumentation so it could be converted and used as OTel instrumentation. @Aneurysm9 is going to follow up with @lizthegrey.
Also,
As there are multiple approaches to provide this instrumentation we should likely add multiple instrumentation packages for users to try. Ideally we will ultimately provide a single well vetted and supported instrumentation library for the
db
package, but until then we should add add an experimental directory and add all 3 (possibly 2) solutions listed here to that directory so they can be iterated on and users can evaluate them.
from opentelemetry-go-contrib.
Looking at that PR, it would be nice to be able to opt into specific spans. The SQL driver can be very chatty, so allowing opt-in like the ocsql package would be nice.
https://github.com/opencensus-integrations/ocsql/blob/master/options.go#L12-L64
from opentelemetry-go-contrib.
For anyone subscribed to or who finds this issue, this fork https://github.com/XSAM/otelsql is active and probably your best bet for the time being.
from opentelemetry-go-contrib.
I'd like to work on this by implementing an MVP (probably not include metrics first)
from opentelemetry-go-contrib.
Is there any progress about this issue?
If we need an approache like opencensus, how about https://github.com/j2gg0s/otsql , used in our other project.
from opentelemetry-go-contrib.
Is there someone is this issue that could head up this effort? This issue has not seen much focus from the maintainer/approver development team of this project as it is not a part of the things needed for our GA release (there are quite a lot of issues prioritized for that effort that need to take precedence).
That said, it seems like there is a fair amount of community interest here. It would be ideal if someone can take the initiative and start work on what @seanschade laid out as a first step to a solutions design:
I think an approach similar to https://github.com/seslattery/otelsql is more desirable than #32.
If we would like to forego the wrapper approach then we have both OpenTracing and OpenCensus examples to draw from.
It would be nice if we could enumerate what we like about each, and what this should look like for OpenTelemetry.
From the outcome of that analysis the implementation of a solution would need to be iterative. We've discussed a few times in our SIG meetings that the initial solution here does not need to solve all the combinatorial possibilities (like #32) but provided more of an MVP to build from.
from opentelemetry-go-contrib.
+1 for @j2gg0s's implementation. otelsql
only has a tracing implementation.
Metrics are far more important to us than tracing as they power alerting, dashboards and automation.
from opentelemetry-go-contrib.
@seslattery please do open a PR. I'd like to see Register()
use functional options for the tracer and attributes, to be more in line with the SDK and other instrumentations. I'm also not sure about using runtime.Caller()
to derive span names from callers rather than just putting static strings at the call site. Overall, though, this is a good start.
from opentelemetry-go-contrib.
@MrAlias what are your thoughts on using something like https://github.com/ngrok/sqlmw as an interceptor to wrap drivers?
I agree with @derekperkins on the approach. I think an approach similar to what OpenCensus did is preferred to the approach of using the sqlmw
approach.
It would be nice if we could enumerate what we like about each, and what this should look like for OpenTelemetry.
This sounds like a great idea 👍 . I think including something like this in this issue would help future developers understand the eventual choice made and what out strategy was.
from opentelemetry-go-contrib.
In accordance with the new instrumentation policy hosting guidelines I'm closing this.
from opentelemetry-go-contrib.
Prior attempt within this repo: #32
from opentelemetry-go-contrib.
@seanschade Thanks for the ping! Sorry for the delay, this fell off my radar. I cleaned up the code a little bit and added an example. PR is here: #320. Notably my implementation still doesn't have any tests.
from opentelemetry-go-contrib.
Are alternative designs being considered? ocsql
, otelsql
(which is based on that), and instrumentedsql
(OpenTracing option, e.g. https://github.com/luna-duclos/instrumentedsql) are all very verbose in the number of spans that are created (as @derekperkins pointed out), most of which are of low value by themselves (e.g. 'rows next'). While it is possible to disable creation of spans for specific things, instrumenting this way may not be the most effective way to make things observable?
Using an observability tool like Honeycomb/Lightstep, you want as many details available on the one span as possible as that allows those things to be properly queried, and expose where other tags differ on those kinds of spans. The way I imagine that would be something like:
name: "SELECT"
tags:
# Use proper query text per OTel spec
db.statement: "SELECT ... FROM ... WHERE ..."
# Statement duration before iterating over rows
db.statement_duration_ms: 38.2
# Need to be considered carefully, given args may include sensitive data like PII, or could have large values
db.arg.foo: "bar"
db.arg.fizz: "buzz"
# Could be accumulated as the calling code iterates over the rows, added on `rows.Close()`
db.rows_returned: 23
...
# Spec tags that apply to all DB spans
db.system: "postgresql"
db.user: "username"
db.name: "Customers"
...
I'd also be looking to hook into these details to aggregate things like number of statements executed, rows returned, total duration to put into the root span for the service e.g. the HTTP request that needs those queries executed.
from opentelemetry-go-contrib.
I've been watching this issue, but I wasn't aware that @XSAM had indeed built a version https://github.com/XSAM/otelsql. In his PR, @Aneurysm9 said this about landing official support #505 (comment)
We've discussed this a couple times at the SIG meeting and the consensus seems to be that we are not ready to land a db/sql instrumentation in the contrib repo at this time. We would like to see broader usage and community consensus on an approach before we commit to the level of support that would be required of a package in contrib.
from opentelemetry-go-contrib.
FYI initial attempt that looks like a different approach would be more desirable #32
from opentelemetry-go-contrib.
I have an initial proof of concept I implemented at https://github.com/seslattery/otelsql. If this seems like a useful direction to be explored more, I'd be happy to open a PR and work on polishing it up some more.
from opentelemetry-go-contrib.
Prior work in OpenCensus might be useful to look at: https://github.com/opencensus-integrations/ocsql
I would not copy this, but might be interesting to look at.
from opentelemetry-go-contrib.
Also: https://github.com/DataDog/dd-trace-go/tree/v1.24.1/contrib/database/sql
from opentelemetry-go-contrib.
@seslattery are you still planning on submitting a PR?
I have an initial proof of concept I implemented at https://github.com/seslattery/otelsql. If this seems like a useful direction to be explored more, I'd be happy to open a PR and work on polishing it up some more.
@MrAlias what are your thoughts on using something like https://github.com/ngrok/sqlmw as an interceptor to wrap drivers?
I think an approach similar to https://github.com/seslattery/otelsql is more desirable than #32.
If we would like to forego the wrapper approach then we have both OpenTracing and OpenCensus examples to draw from.
It would be nice if we could enumerate what we like about each, and what this should look like for OpenTelemetry.
from opentelemetry-go-contrib.
hi, any progress about this issue?
from opentelemetry-go-contrib.
+1 for @j2gg0s's implementation.
otelsql
only has a tracing implementation.Metrics are far more important to us than tracing as they power alerting, dashboards and automation.
This solution would meet my needs. Could @j2gg0s solution be the MVP, and would it move into the contrib repo?
from opentelemetry-go-contrib.
Any update on the SIG discussion?
from opentelemetry-go-contrib.
from opentelemetry-go-contrib.
Has anyone done a comparison between https://github.com/XSAM/otelsql and https://github.com/uptrace/opentelemetry-go-extra/ ?
from opentelemetry-go-contrib.
Related Issues (20)
- Make `otelhttp` produce old, new and duplicate Client Trace Attributes
- otelslog: Limit the size of kvBuffer returned to the pool
- Make otelhttp produce old, new and duplicate Metrics
- Logger support in otelhttp HOT 1
- Allow to carry-over a traceparent from another library HOT 2
- config: add support filtering resource attributes as constant labels
- Add per quest customized attribute to http metrics instrumentation
- prometheusbridge: Creates invalid exemplars for Prometheus counters HOT 2
- Request for new component: Baggage Span Processor
- Add a resource detector for physical hosts
- otelmux middleware doesn't support websockets
- Add Changelog to advertise the availaibty of server semconv HOT 1
- Add zerolog log bridge
- Add a resource detector for Azure VMs
- Add a resource detector that sets a default value for `service.instance.id` according to semantic conventions HOT 2
- Log Bridge returning io.Writer HOT 6
- Use a `kvBuffer` in logrus bridge
- Does span execute span. end HOT 2
- otelgrpc: add WithSpanOptions behavior to stats_handler
- config: add stdout exporter for log signal HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from opentelemetry-go-contrib.