Add ecto adapter attribute #65

jhonndabi · 2022-02-09T22:19:39Z

I couldn't find any discussion about how to include the required attribute db.system.
I think currently there isn't a good way to infer that information, maybe ecto adapters can send to telemetry that information through metadata. For now, if we simple pass the ecto adapter as attribute, will be possible to infer and map that info.
Elixir.Ecto.Adapters.Postgres -> postgresql
Elixir.Ecto.Adapters.MyXQL -> mysql
Elixir.Ecto.Adapters.Tds -> mssql

This PR do the following:

Change some attributes key to follow semantic conventions.
Add the attribute ecto.db.adapter
Add the attribute db.system

linux-foundation-easycla · 2022-02-09T22:19:43Z

The committers listed above are authorized under a signed CLA.

✅ login: jhonndabi / name: Jonadabe Nascimento (2f77ab9, f724c9b, b638a1d, 725a891, f9817ba, 40cf83e, e076e65, f8ff0bb)

tsloughter · 2022-02-09T22:58:53Z

Could submit the ecto specific adapter attribute to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes-for-specific-technologies -- as far as I can tell it is the same as like a JDBC driver class name.

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex

tsloughter · 2022-02-09T23:01:28Z

I agree you can create a function to map the adapter name to the db.system name and add db.system attribute to this.

bryannaegele · 2022-02-09T23:06:08Z

Can you open an issue on ecto to request it be added as an attribute in the meta?

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex

bryannaegele · 2022-02-09T23:21:55Z

Could submit the ecto specific adapter attribute to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes-for-specific-technologies -- as far as I can tell it is the same as like a JDBC driver class name.

I think this is supposed to be at the driver level which would be postgrex for the postgres adapter.

jhonndabi · 2022-02-10T16:26:59Z

Could submit the ecto specific adapter attribute to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes-for-specific-technologies -- as far as I can tell it is the same as like a JDBC driver class name.

I think this is supposed to be at the driver level which would be postgrex for the postgres adapter.

Driver is a required option to sql adapter.

https://github.com/elixir-ecto/ecto_sql/blob/6935c289f68e1404ef0a6ec652d8fb1f89a82b96/lib/ecto/adapters/sql.ex#L88
https://github.com/elixir-ecto/ecto_sql/blob/6935c289f68e1404ef0a6ec652d8fb1f89a82b96/lib/ecto/adapters/sql.ex#L735

What do you think to ask them in the issue I will open to include driver and dbms_identifier in the telemetry metadata?

cc @bryannaegele @tsloughter

tsloughter · 2022-03-16T19:39:50Z

@jhonndabi sorry I missed this, that sounds like a good plan to me.

tsloughter · 2022-03-16T19:44:14Z

Also,j can you change the attribute to ecto.db.adapter.

tsloughter · 2022-03-26T16:18:18Z

@jhonndabi would you be ok adding db.system with just the mapping you mention in your first comment? Since that information is available, I don't see why we don't just do the mapping.

jhonndabi · 2022-04-01T16:52:26Z

@tsloughter Ok, Sorry for the delay, I didn't see before. I'll work on that, and do the mapping.

tsloughter · 2022-11-06T11:31:42Z

@jhonndabi can you resolve the conflicts?

I think this is good to merge now once the conflicts are resolved.

jhonndabi · 2023-06-24T12:05:15Z

@tsloughter yes, of course! Sorry the delay. : )

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex

instrumentation/opentelemetry_ecto/test/opentelemetry_ecto_test.exs

tsloughter · 2023-06-24T22:59:12Z

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex

      "db.name": database,
-      "db.url": url,
+      "db.sql.table": source,


We don't actually know that an Ecto source is a sql table, do we here?

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.

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).

It could be a view, for example, not sure if it would break your semantics.

I don't think it's breaks the semantics here, view is like a virtual table.

jhonndabi · 2023-07-25T17:40:23Z

@tsloughter Is there something else I can do here to get this merged?

tsloughter · 2023-10-31T20:10:11Z

@jhonndabi hey, sorry, can you rebase?

And @yordis does this overlap with your PR that is still a draft?

yordis · 2023-10-31T21:06:44Z

@tsloughter yeah ... that is why I kept it that way, we can merge this one and I can continue the work there (either close it or change it)

yordis · 2023-10-31T21:13:00Z

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex

+
+  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)


@tsloughter leaving a note here, as I said before, it will be prudent to ask Ecto folks to include a function under the adapter module that allow us to fetch the engine name ...

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

yordis · 2023-10-31T21:13:43Z

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex

+    * `: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`


Would this be globally or per Adapter bases?

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).

tsloughter · 2023-11-11T10:56:27Z

@jhonndabi can you rebase this?

jhonndabi · 2023-11-11T11:12:52Z

@tsloughter done : )

tsloughter · 2024-10-31T09:25:45Z

Sorry this never got in. This needs another rebase at least.

bryannaegele · 2024-11-01T21:11:18Z

@tsloughter the whole lib needs a semconv rewrite. Probably best to leave it for that PR

jhonndabi requested review from bryannaegele and tsloughter as code owners February 9, 2022 22:19

github-actions bot added elixir instrumentation opentelemetry_ecto labels Feb 9, 2022

tsloughter reviewed Feb 9, 2022

View reviewed changes

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex Outdated Show resolved Hide resolved

bryannaegele reviewed Feb 9, 2022

View reviewed changes

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex Outdated Show resolved Hide resolved

instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex Outdated Show resolved Hide resolved

jhonndabi requested review from tsloughter and bryannaegele April 1, 2022 19:53

jhonndabi force-pushed the add-ecto-adapter-attribute branch from e3b7d11 to d4f3a89 Compare June 24, 2023 11:56

tsloughter reviewed Jun 24, 2023

View reviewed changes

github-actions bot added the scope-ci label Jul 6, 2023

jhonndabi force-pushed the add-ecto-adapter-attribute branch from 6444296 to f6e4e5c Compare July 6, 2023 23:14

tsloughter mentioned this pull request Jul 7, 2023

chore: add sc to ecto #142

Draft

jhonndabi force-pushed the add-ecto-adapter-attribute branch from f6e4e5c to 043e438 Compare July 17, 2023 17:44

yordis reviewed Oct 31, 2023

View reviewed changes

jhonndabi added 8 commits November 24, 2023 08:19

change attributes key to follow the semantic convention

2f77ab9

add ecto adapter attribute

f724c9b

Drop db.connection_string attribute from opentelemetry_ecto

b638a1d

Drop db.type attribute from opentelemetry_ecto

725a891

add db system attribute

f9817ba

fix: rebase modifications and format

40cf83e

chore: change db_systems with semantic_conventions updated table

e076e65

chore: add mariadb service to ci workflow

f8ff0bb

jhonndabi force-pushed the add-ecto-adapter-attribute branch from 527511c to f8ff0bb Compare November 24, 2023 11:38

jhonndabi requested review from tsloughter, v0idpwn and yordis November 24, 2023 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ecto adapter attribute #65

Add ecto adapter attribute #65

jhonndabi commented Feb 9, 2022 •

edited

Loading

linux-foundation-easycla bot commented Feb 9, 2022 •

edited

Loading

tsloughter commented Feb 9, 2022

tsloughter commented Feb 9, 2022

bryannaegele commented Feb 9, 2022

bryannaegele commented Feb 9, 2022

jhonndabi commented Feb 10, 2022 •

edited

Loading

tsloughter commented Mar 16, 2022

tsloughter commented Mar 16, 2022

tsloughter commented Mar 26, 2022

jhonndabi commented Apr 1, 2022 •

edited

Loading

tsloughter commented Nov 6, 2022

jhonndabi commented Jun 24, 2023

tsloughter Jun 24, 2023

jhonndabi Jul 6, 2023

tsloughter Jul 7, 2023

v0idpwn Jul 25, 2023

jhonndabi Nov 11, 2023

jhonndabi commented Jul 25, 2023

tsloughter commented Oct 31, 2023

yordis commented Oct 31, 2023

yordis Oct 31, 2023

yordis Oct 31, 2023

jhonndabi Nov 11, 2023

tsloughter commented Nov 11, 2023

jhonndabi commented Nov 11, 2023

tsloughter commented Oct 31, 2024

bryannaegele commented Nov 1, 2024

Add ecto adapter attribute #65

Are you sure you want to change the base?

Add ecto adapter attribute #65

Conversation

jhonndabi commented Feb 9, 2022 • edited Loading

linux-foundation-easycla bot commented Feb 9, 2022 • edited Loading

tsloughter commented Feb 9, 2022

tsloughter commented Feb 9, 2022

bryannaegele commented Feb 9, 2022

bryannaegele commented Feb 9, 2022

jhonndabi commented Feb 10, 2022 • edited Loading

tsloughter commented Mar 16, 2022

tsloughter commented Mar 16, 2022

tsloughter commented Mar 26, 2022

jhonndabi commented Apr 1, 2022 • edited Loading

tsloughter commented Nov 6, 2022

jhonndabi commented Jun 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhonndabi commented Jul 25, 2023

tsloughter commented Oct 31, 2023

yordis commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsloughter commented Nov 11, 2023

jhonndabi commented Nov 11, 2023

tsloughter commented Oct 31, 2024

bryannaegele commented Nov 1, 2024

jhonndabi commented Feb 9, 2022 •

edited

Loading

linux-foundation-easycla bot commented Feb 9, 2022 •

edited

Loading

jhonndabi commented Feb 10, 2022 •

edited

Loading

jhonndabi commented Apr 1, 2022 •

edited

Loading