Adopt distributed tracing

[ptoffy]

These changes are now available in 1.55.0

This adopts tracing on DatabaseQuery, only using async/await interfaces and not on raw/sql-kit queries as those require tracing support in SQLKit or lower level packages like PostgresKit or MySQLNIO.
This relies on #647

[slashmo]

@ptoffy How do you expect this to look like once database drivers like postgres-nio gain tracing support? I'd assume they'd create child spans of the fluent spans? In that case I'm not sure we should follow the OTel semantic conventions for the Fluent spans at all. I looked at gorm's tracing support and they e.g. add gorm-specific spans rather than the underlying DB spans: https://github.com/go-gorm/opentelemetry

[ptoffy]

@slashmo Yes my idea is to create child spans in the underlying drivers but that might still be a while off since I'm pretty sure those are all event loop based (see https://github.com/vapor/fluent-postgres-driver). Maybe @gwynne has some insight into this.
Is there a reason you want to follow gorm instead of the OTel convention? I'd be more inclined to follow the spec since there is one

[slashmo]

Is there a reason you want to follow gorm instead of the OTel convention? I'd be more inclined to follow the spec since there is one

I'm all for following the spec. However, there isn't one for ORMs, only for db drivers. So if we'd follow the DB driver conventions here we'd have duplicate spans between Fluent and the drivers. Gorm is just one example ORM where I found built-in tracing support, and they don't follow the DB driver conventions.

[ptoffy]

@slashmo what do you think about emitting driver-agnostic traces (like the ones that are in the PR now) in fluent-kit and leaving db-specific traces up to the drivers to implement? The drivers rely on fluent-kit anyway so you'll always have both. The only pain about this approach I can think of is keeping track of which ones are where, but we can just decide it upfront

[slashmo]

Depending on how strict observability backends are about the semantic conventions I could see this being a problem. For example, if they look for certain required attributes that we don't supply here I could imagine it breaking their visualization of database spans. I think I'd slightly be in favor of using non-spec operation name and attributes for FluentKit spans for that reason. I also think it shouldn't be up to the Vapor DB drivers to add spans but the underlying database libraries agnostic from Vapor, like postgres-nio. This way we'd also benefit from these spans in apps not using Vapor.

[ptoffy]

While I do agree and did update the attributes to use fluent specific names, I did notice that gorm also uses the semantic conventions' attributes, here for example https://github.com/go-gorm/opentelemetry/blob/master/tracing/tracing.go#L264-L278. Anyway let me know what you think of the current ones

[0xTim]

Can we add more tests for:

• getting relations
• queries on relations
• first
• min
• plain aggregate call
• raw query

[ptoffy]

Raw query is not currently traced since that requires tracing in SQLKit and/or in DBMS specific underlying -Kit library

[0xTim]

Ah yeah fair enough, we should look at adopting that after this, especially with PostgresNIO and see if we can migrate to the new Postgres stuff

[gwynne]

Migrating to PostgresClient is severely problematic because it's not really compatible with an ELF-based interface. One of the bigger issues is that it would require specific support in the fluent package to hook into the Application lifecycle, since in Vapor 4 there's no expectation of a ServiceGroup to attach a PostgresClient to. Except that such logic should really live in fluent-postgres-driver, but it can't because that's independent of Vapor. And so on. There are other issues also. It's not necesarily insurmountable, but my general mode of thought has been "let's just do pooling correctly in Fluent 5 and people who need PostgresClient in the meantime can use it with PostgresKit" (since PostgresKit now includes instructions for using it to replace the AsyncKit pooling; it's just that those instructions don't map onto something the FluentKit layer can do).

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 26.48%. Comparing base (eb3bd5b) to head (28901e5).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...urces/FluentKit/Query/Database/DatabaseQuery.swift	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
+ Coverage   25.83%   26.48%   +0.64%     
==========================================
  Files         148      148              
  Lines        8852     8869      +17     
==========================================
+ Hits         2287     2349      +62     
+ Misses       6565     6520      -45     

Files with missing lines	Coverage Δ
...s/FluentKit/Concurrency/Database+Concurrency.swift	29.41% <100.00%> (+4.41%)	⬆️
...rces/FluentKit/Concurrency/Model+Concurrency.swift	89.31% <ø> (ø)
Sources/FluentKit/Database/Database+Logging.swift	32.25% <100.00%> (+2.25%)	⬆️
Sources/FluentKit/Database/Database.swift	85.00% <100.00%> (+0.78%)	⬆️
Sources/FluentKit/Database/Databases.swift	60.00% <100.00%> (+0.42%)	⬆️
Sources/FluentKit/Query/Builder/QueryBuilder.swift	74.85% <100.00%> (ø)
...urces/FluentKit/Query/Database/DatabaseQuery.swift	34.78% <92.85%> (+13.35%)	⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[0xTim]

LGTM - if @slashmo has any other feedback that would be great, otherwise I'll let @gwynne have a review. Though this one should be fairly uncontroversial

[0xTim]

PS looks like the tests will need to be wrapped in a 6.1 check since traits aren't supported in 6.0

[gwynne]

One very tiny nit, otherwise lgtm.