-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add ecto adapter attribute #65
base: main
Are you sure you want to change the base?
Changes from all commits
2f77ab9
f724c9b
b638a1d
725a891
f9817ba
40cf83e
e076e65
f8ff0bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,26 @@ | ||
import Config | ||
|
||
config :opentelemetry_ecto, | ||
ecto_repos: [OpentelemetryEcto.TestRepo] | ||
ecto_repos: [ | ||
OpentelemetryEcto.MariaDBTestRepo, | ||
OpentelemetryEcto.TestRepo | ||
] | ||
|
||
config :opentelemetry_ecto, OpentelemetryEcto.TestRepo, | ||
hostname: "localhost", | ||
username: "postgres", | ||
password: "postgres", | ||
database: "opentelemetry_ecto_test", | ||
pool: Ecto.Adapters.SQL.Sandbox | ||
pool: Ecto.Adapters.SQL.Sandbox, | ||
telemetry_prefix: [:opentelemetry_ecto, :test_repo] | ||
|
||
config :opentelemetry_ecto, OpentelemetryEcto.MariaDBTestRepo, | ||
hostname: "localhost", | ||
username: "mariadb", | ||
password: "mariadb", | ||
database: "opentelemetry_ecto_test", | ||
pool: Ecto.Adapters.SQL.Sandbox, | ||
telemetry_prefix: [:opentelemetry_ecto, :mariadb_test_repo] | ||
|
||
config :opentelemetry, | ||
processors: [{:otel_batch_processor, %{scheduled_delay_ms: 1}}] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,61 @@ defmodule OpentelemetryEcto do | |
|
||
require OpenTelemetry.Tracer | ||
|
||
@db_systems [ | ||
"other_sql", | ||
"mssql", | ||
"mssqlcompact", | ||
"mysql", | ||
"oracle", | ||
"db2", | ||
"postgresql", | ||
"redshift", | ||
"hive", | ||
"cloudscape", | ||
"hsqldb", | ||
"progress", | ||
"maxdb", | ||
"hanadb", | ||
"ingres", | ||
"firstsql", | ||
"edb", | ||
"cache", | ||
"adabas", | ||
"firebird", | ||
"derby", | ||
"filemaker", | ||
"informix", | ||
"instantdb", | ||
"interbase", | ||
"mariadb", | ||
"netezza", | ||
"pervasive", | ||
"pointbase", | ||
"sqlite", | ||
"sybase", | ||
"teradata", | ||
"vertica", | ||
"h2", | ||
"coldfusion", | ||
"cassandra", | ||
"hbase", | ||
"mongodb", | ||
"redis", | ||
"couchbase", | ||
"couchdb", | ||
"cosmosdb", | ||
"dynamodb", | ||
"neo4j", | ||
"geode", | ||
"elasticsearch", | ||
"memcached", | ||
"cockroachdb", | ||
"opensearch", | ||
"clickhouse", | ||
"spanner", | ||
"trino" | ||
] | ||
|
||
@doc """ | ||
Attaches the OpentelemetryEcto handler to your repo events. This should be called | ||
from your application behaviour on startup. | ||
|
@@ -44,6 +99,10 @@ defmodule OpentelemetryEcto do | |
sanitized version of it. This is useful for removing sensitive information from the | ||
query string. Unless this option is `:enabled` or a function, | ||
query statements will not be recorded on spans. | ||
* `:db_system` - The identifier for the database management system (DBMS). | ||
defaults to the mapped value of the ecto adapter used. | ||
Must follow the list of well-known db systems from semantic conventions. | ||
See `https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md` | ||
""" | ||
def setup(event_prefix, config \\ []) do | ||
event = event_prefix ++ [:query] | ||
|
@@ -54,7 +113,7 @@ defmodule OpentelemetryEcto do | |
def handle_event( | ||
event, | ||
measurements, | ||
%{query: query, source: source, result: query_result, repo: repo, type: type}, | ||
%{query: query, source: source, result: query_result, repo: repo}, | ||
config | ||
) do | ||
# Doing all this even if the span isn't sampled so the sampler | ||
|
@@ -64,16 +123,7 @@ defmodule OpentelemetryEcto do | |
end_time = :opentelemetry.timestamp() | ||
start_time = end_time - total_time | ||
database = repo.config()[:database] | ||
|
||
url = | ||
case repo.config()[:url] do | ||
nil -> | ||
# TODO: add port | ||
URI.to_string(%URI{scheme: "ecto", host: repo.config()[:hostname]}) | ||
|
||
url -> | ||
url | ||
end | ||
adapter = repo.__adapter__() | ||
|
||
span_prefix = | ||
case Keyword.fetch(config, :span_prefix) do | ||
|
@@ -87,20 +137,13 @@ defmodule OpentelemetryEcto do | |
time_unit = Keyword.get(config, :time_unit, :microsecond) | ||
additional_attributes = Keyword.get(config, :additional_attributes, %{}) | ||
|
||
db_type = | ||
case type do | ||
:ecto_sql_query -> :sql | ||
_ -> type | ||
end | ||
|
||
# TODO: need connection information to complete the required attributes | ||
# net.peer.name or net.peer.ip and net.peer.port | ||
base_attributes = %{ | ||
"db.type": db_type, | ||
source: source, | ||
"db.instance": database, | ||
"ecto.db.adapter": to_string(adapter), | ||
"db.system": db_system(config[:db_system], adapter), | ||
"db.name": database, | ||
"db.url": url, | ||
"db.sql.table": source, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't actually know that an Ecto source is a sql table, do we here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://hexdocs.pm/ecto/Ecto.Schema.Metadata.html#module-source The doc says it will be a table or a collection, I don't know if we have a way to determine if it's a table, but it could be a table. I'm not sure if for SQL adapters it will be always a table. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figure for a sql adapter it will alway be a table. So thats all we need to know to know to include it (I think). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be a view, for example, not sure if it would break your semantics. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's breaks the semantics here, view is like a virtual table. |
||
"total_time_#{time_unit}s": System.convert_time_unit(total_time, :native, time_unit) | ||
} | ||
|
||
|
@@ -110,7 +153,6 @@ defmodule OpentelemetryEcto do | |
base_attributes | ||
|> add_measurements(measurements, time_unit) | ||
|> maybe_add_db_statement(db_statement_config, query) | ||
|> maybe_add_db_system(repo.__adapter__()) | ||
|> add_additional_attributes(additional_attributes) | ||
|
||
parent_context = | ||
|
@@ -189,27 +231,15 @@ defmodule OpentelemetryEcto do | |
attributes | ||
end | ||
|
||
defp maybe_add_db_system(attributes, Ecto.Adapters.Postgres) do | ||
Map.put(attributes, :"db.system", :postgresql) | ||
end | ||
|
||
defp maybe_add_db_system(attributes, Ecto.Adapters.MyXQL) do | ||
Map.put(attributes, :"db.system", :mysql) | ||
end | ||
|
||
defp maybe_add_db_system(attributes, Ecto.Adapters.SQLite3) do | ||
Map.put(attributes, :"db.system", :sqlite) | ||
end | ||
|
||
defp maybe_add_db_system(attributes, Ecto.Adapters.Tds) do | ||
Map.put(attributes, :"db.system", :mssql) | ||
end | ||
|
||
defp maybe_add_db_system(attributes, _) do | ||
attributes | ||
end | ||
|
||
defp add_additional_attributes(attributes, additional_attributes) do | ||
Map.merge(attributes, additional_attributes) | ||
end | ||
|
||
defp db_system(db_system) when db_system in @db_systems, do: db_system | ||
defp db_system(_), do: "other_sql" | ||
|
||
defp db_system(nil, Ecto.Adapters.Postgres), do: "postgresql" | ||
defp db_system(nil, Ecto.Adapters.MyXQL), do: "mysql" | ||
defp db_system(nil, Ecto.Adapters.Tds), do: "mssql" | ||
defp db_system(db_system, _), do: db_system(db_system) | ||
Comment on lines
+237
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tsloughter leaving a note here, as I said before, it will be prudent to ask I am not sure how open they will be, but I think it is a fair thing to ask and see what happens. "Ideally," this package wouldn't maintain the mapping |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
defmodule OpentelemetryEcto.MariaDBTestRepo.Migrations.SetupTables do | ||
use Ecto.Migration | ||
|
||
def change do | ||
create table(:users) do | ||
add :email, :string | ||
end | ||
|
||
create table(:posts) do | ||
add :body, :text | ||
add :user_id, references(:users) | ||
end | ||
|
||
create table(:comments) do | ||
add :body, :text | ||
add :user_id, references(:users) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
defmodule OpentelemetryEcto.MariaDBTestRepo do | ||
use Ecto.Repo, | ||
otp_app: :opentelemetry_ecto, | ||
adapter: Ecto.Adapters.MyXQL | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
defmodule OpentelemetryEcto.TestRepo do | ||
use Ecto.Repo, | ||
otp_app: :opentelemetry_ecto, | ||
adapter: Ecto.Adapters.Postgres, | ||
telemetry_prefix: [:opentelemetry_ecto, :test_repo] | ||
adapter: Ecto.Adapters.Postgres | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
OpentelemetryEcto.MariaDBTestRepo.start_link() | ||
OpentelemetryEcto.TestRepo.start_link() | ||
|
||
ExUnit.start(capture_log: true) | ||
|
||
Ecto.Adapters.SQL.Sandbox.mode(OpentelemetryEcto.MariaDBTestRepo, {:shared, self()}) | ||
Ecto.Adapters.SQL.Sandbox.mode(OpentelemetryEcto.TestRepo, {:shared, self()}) |
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 this be globally or per Adapter bases?
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.
Per adapter bases. I think
OpentelemetryEcto.setup/2
is meant to use just for one repo, right? And each repo can have only one Adapter (AFAIK).