opentelemetry-erlang-contrib icon indicating copy to clipboard operation
opentelemetry-erlang-contrib copied to clipboard

Add ecto adapter attribute

Open jhonndabi opened this issue 3 years ago • 17 comments

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

jhonndabi avatar Feb 09 '22 22:02 jhonndabi

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: jhonndabi / name: Jonadabe Nascimento (2f77ab9072d36900e98357e10cab6f4c96b85250, f724c9b1aab6a87e6ae8e51134fdf6639aa93208, b638a1d790bbb8ad53ae000402cb4cd9d9c08831, 725a891035c4babe493fe0702bdcdc5036e510c7, f9817ba34cd9eec232b06f48f5f50daa82222c16, 40cf83ec96cb8527c7e885f12eedf8d001ce5235, e076e65d9db1579a6c88c3bc2e992effd8c26f70, f8ff0bbb4b93d7e71bac8336406c9440e03f9737)

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.

tsloughter avatar Feb 09 '22 22:02 tsloughter

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

tsloughter avatar Feb 09 '22 23:02 tsloughter

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

bryannaegele avatar Feb 09 '22 23:02 bryannaegele

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.

bryannaegele avatar Feb 09 '22 23:02 bryannaegele

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

jhonndabi avatar Feb 10 '22 16:02 jhonndabi

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

tsloughter avatar Mar 16 '22 19:03 tsloughter

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

tsloughter avatar Mar 16 '22 19:03 tsloughter

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

tsloughter avatar Mar 26 '22 16:03 tsloughter

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

jhonndabi avatar Apr 01 '22 16:04 jhonndabi

@jhonndabi can you resolve the conflicts?

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

tsloughter avatar Nov 06 '22 11:11 tsloughter

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

jhonndabi avatar Jun 24 '23 12:06 jhonndabi

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

jhonndabi avatar Jul 25 '23 17:07 jhonndabi

@jhonndabi hey, sorry, can you rebase?

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

tsloughter avatar Oct 31 '23 20:10 tsloughter

@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 avatar Oct 31 '23 21:10 yordis

@jhonndabi can you rebase this?

tsloughter avatar Nov 11 '23 10:11 tsloughter

@tsloughter done : )

jhonndabi avatar Nov 11 '23 11:11 jhonndabi