Semantic conventions - Githubissues

rdooley commented 1 year ago

Getting the ball slowly rolling on moving towards Otel semantic conventions for span names and attributes

Most relevant links are

TODO: testing?

DaniFoldi commented 1 year ago

Hi :wave:,

Thanks for opening this PR. I see that you're trying to push for semantic conventions, however I disagree with their usage in this case. The collected telemetry data will get more vague, and there are also some properties you lost in the conversion. I disagree that all attributes should be called db.cf.<x>, as you lose searchability by name of attribute.

A specific example: Cloudflare also has a PubSub product, entirely separate from Queues. Eventually, that will also get more API in workers, so how will you differentiate a queue trigger from a pubsub trigger, if you use the semantic "queue" trigger type for both? I think it's best to keep the attributes not-entirely-semantic, so they convey more (and more accurate) information.

With all that said, I'm not the maintainer of this project, so @evanderkoogh will have the final say.

rdooley commented 1 year ago

Thanks for opening this PR. I see that you're trying to push for semantic conventions, however I disagree with their usage in this case. The collected telemetry data will get more vague, and there are also some properties you lost in the conversion. I disagree that all attributes should be called db.cf.<x>, as you lose searchability by name of attribute.

I mostly care about the fetch/Http conventions (particularly span name). The KV and DO instrumentations are largely a best first stab.

Would using the existing SemanticAttributes for DB_NAME, DB_SYSTEM, DB_OPERATION etc with the kv.* and do.* attributes becoming e.g. cf.kv.cache_ttl ? In this case the main thing is a desire to have camelCase suffixes become snake_case following the pattern of existing semantic conventions.

A specific example: Cloudflare also has a PubSub product, entirely separate from Queues. Eventually, that will also get more API in workers, so how will you differentiate a queue trigger from a pubsub trigger, if you use the semantic "queue" trigger type for both? I think it's best to keep the attributes not-entirely-semantic, so they convey more (and more accurate) information.

The semantic conventions has a Should in there, so how about queue for queue and pubsub for pubsub?

With all that said, I'm not the maintainer of this project, so @evanderkoogh will have the final say.

Appreciate the feedback here, trying to make this better for all users, not just my specific use case 🙏

rdooley commented 1 year ago

Anyways, here are some spans (I just logged them to console using a ConsoleSpanExporter I'll throw up in a separate PR)

Spans ahead

`/do` hit ``` {"traceId":"bfb51783801c6cc6c113cff3f0d5d029","parentSpanId":"","traceState":"","name":"fetchHandler","id":"373022780407ea12","kind":1,"startTime":1694704986097,"endTime":1694704986172,"duration":75,"attributes":{"faas.trigger":"http","faas.coldstart":true,"faas.execution":"8069a112fbab3075","http.method":"GET","http.url":"https://otel-test.shopify-hackathon8867.workers.dev/favicon.ico","http.host":"otel-test.shopify-hackathon8867.workers.dev","http.scheme":"https:","http.target":"/favicon.ico","http.route":"/favicon.ico","http.user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/116.0.0.0 Safari/537.36","http.request_content_length":null,"http.request_content-type":null,"http.accepts":null,"net.colo":"SEA","net.country":"US","net.request_priority":"","net.tls_cipher":"ECDHE-RSA-AES128-GCM-SHA256","net.tls_version":"TLSv1.2","net.asn":209,"net.tcp_rtt":1,"greeting":"G'day World","http.status_code":200,"http.response_content_length":null,"http.response_content-type":"text/plain;charset=UTF-8"},"status":{"code":1},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"bfb51783801c6cc6c113cff3f0d5d029","parentSpanId":"373022780407ea12","traceState":"","name":"fetch: workers.dev","id":"4592334286a898ec","kind":2,"startTime":1694704986172,"endTime":1694704986338,"duration":166,"attributes":{"http.method":"GET","http.url":"https://workers.dev/","http.host":"workers.dev","http.scheme":"https:","http.target":"/","http.route":"/","http.user_agent":null,"http.request_content_length":null,"http.request_content-type":null,"http.accepts":null,"http.status_code":200,"http.response_content_length":null,"http.response_content-type":"text/html; charset=utf-8"},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"cb06ec7eb1661e5203ce209e917d87cc","parentSpanId":"ac55e137de49d65a","traceState":"","name":"do:storage:get","id":"01ee797c8e61613f","kind":2,"startTime":1694704986996,"endTime":1694704986996,"duration":0,"attributes":{"operation":"get","db.cf.key":"something","db.cf.hasResult":true},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings-do","service.namespace":"","service.version":""},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings-do","service.namespace":"","service.version":""},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"cb06ec7eb1661e5203ce209e917d87cc","parentSpanId":"ac55e137de49d65a","traceState":"","name":"do:storage:get","id":"038ad99dd975f7bc","kind":2,"startTime":1694704986996,"endTime":1694704986998,"duration":2,"attributes":{"operation":"get","db.cf.key":"something_else","db.cf.hasResult":false},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud .region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings-do","service.namespace":"","service.version":""},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings-do","service.namespace":"","service.version":""},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"cb06ec7eb1661e5203ce209e917d87cc","parentSpanId":"","traceState":"","name":"do.alarmHandler:","id":"ac55e137de49d65a","kind":0,"startTime":1694704986996,"endTime":1694704986998,"duration":2,"attributes":{"faas.coldstart":false,"do.id":"f47ae871181ef6acfb0ea8d59d7c2e256963d11ee879963506867d6c97c46e69"},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings-do","service.namespace":"","service.version":""},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings-do","service.namespace":"","service.version":""},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} ``` `/` classic flavor ``` {"traceId":"5875cfdcf96742b6b4db1625840286d5","parentSpanId":"","traceState":"","name":"fetchHandler","id":"2b37837195879ed4","kind":1,"startTime":1694704967649,"endTime":1694704967876,"duration":227,"attributes":{"faas.trigger":"http","faas.coldstart":true,"faas.execution":"8069a09faf9a3075","http.method":"GET","http.url":"https://otel-test.shopify-hackathon8867.workers.dev/","http.host":"otel-test.shopify-hackathon8867.workers.dev","http.scheme":"https:","http.target":"/","http.route":"/","http.user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/116.0.0.0 Safari/537.36","http.request_content_length":null,"http.request_content-type":null,"http.accepts":null,"net.colo":"SEA","net.country":"US","net.request_priority":"","net.tls_cipher":"ECDHE-RSA-AES128-GCM-SHA256","net.tls_version":"TLSv1.2","net.asn":209,"net.tcp_rtt":1,"greeting":"G'day World","http.status_code":200,"http.response_content_length":null,"http.response_content-type":"text/plain;charset=UTF-8"},"status":{"code":1},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.nam espace":"","service.version":"0.1"},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"5875cfdcf96742b6b4db1625840286d5","parentSpanId":"2b37837195879ed4","traceState":"","name":"fetch: workers.dev","id":"7cd59b09883509bb","kind":2,"startTime":1694704967876,"endTime":1694704968395,"duration":519,"attributes":{"http.method":"GET","http.url":"https://workers.dev/","http.host":"workers.dev","http.scheme":"https:","http.target":"/","http.route":"/","http.user_agent":null,"http.request_content_length":null,"http.request_content-type":null,"http.accepts":null,"http.status_code":200,"http.response_content_length":null,"http.response_content-type":"text/html; charset=utf-8"},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"asyncAttributesPending":false,"_sy ncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sd k.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"069a8fdc1268038aee86f34b0af3265c","parentSpanId":"5e5060b2e70fa69e","traceState":"","name":"fetch: cloudflare.com","id":"4ce598a54f600f49","kind":2,"startTime":1694704967968,"endTime":1694704968062,"duration":94,"attributes":{"http.method":"GET","http.url":"https://cloudflare.com/","http.host":"cloudflare.com","http.scheme":"https:","http.target":"/","http.route":"/","http.user_agent":null,"http.request_content_length":null,"http.request_content-type":null,"http.accepts":null,"http.status_code":200,"http.response_content_length":null,"http.response_content-type":"text/html; charset=utf-8"},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.s dk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0," droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"069a8fdc1268038aee86f34b0af3265c","parentSpanId":"5e5060b2e70fa69e","traceState":"","name":"cache:stuff:match","id":"8ef0ad13b2ba1093","kind":2,"startTime":1694704968062,"endTime":1694704968072,"duration":10,"attributes":{"cache.name":"stuff","http.url":"https://no-exist.com/","cache.operation":"match","cache.hit":true},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlags":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platf orm":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"asyncAttributesPending":false,"_syncAttributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0,"droppedLinksCount":0} {"traceId":"069a8fdc1268038aee86f34b0af3265c","parentSpanId":"5e5060b2e70fa69e","traceState":"","name":"OTEL_TEST get","id":"b073db57d8d31f31","kind":2,"startTime":1694704968062,"endTime":1694704968082,"duration":20,"attributes":{"binding_type":"KV","db.name":"OTEL_TEST","db.system":"Cloudflare KV","db.operation":"get","db.statement":"get non-existant","db.cf.key":"non-existant","db.cf.has_result":false},"status":{"code":0},"events":[],"ended":true,"links":[],"instrumentationLibrary":{"name":"@microlabs/otel-cf-workers"},"traceFlag s":1,"resource":{"_attributes":{"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScri pt","telemetry.sdk.name":"@microlabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"asyncAttributesPending":false,"_syncAttributes": {"cloud.provider":"cloudflare","cloud.platform":"cloudflare.workers","cloud.region":"earth","faas.max_memory":128,"telemetry.sdk.language":"JavaScript","telemetry.sdk.name":"@micr olabs/otel-workers-sdk","service.name":"greetings","service.namespace":"","service.version":"0.1"},"_asyncAttributesPromise":""},"droppedAttributesCount":0,"droppedEventsCount":0, "droppedLinksCount":0} ```

rdooley commented 1 year ago

@DaniFoldi and @evanderkoogh keen to hear any feedback on this if you have any

plantfansam commented 1 year ago

I agree that we should fully qualify the product in the attribute keys after cf... (so db.cf.kv.foobar = 'wat'). I think this would resolve your concerns about searchability @DaniFoldi?

Here's a general pitch in favor of semantic conventions:

Adhering to semantic conventions will make it so that o11y software (OSS projects and vendors) can support Cloudflare worker trace analysis out-of-the-box. e.g. if a vendor has a generic "database performance" page with logic to perform p99s per span with db.statement calls, Cloudflare spans will be analyzeable with no modification whatsoever.
If we stabilize on some good Cloudflare semantic conventions in this repo (which will necessarily entail the db.cf... prefix), then we can submit them upstream and have them included in this page here. That will let OSS projects and vendors build custom Cloudflare trace analysis tooling on stable APIs.

evanderkoogh commented 1 year ago

Hey @plantfansam, I am certainly very interested in your opinions on this, seeing as you have a ton more experience with the spec and with integrating that with query solutions. So thanks for chipping in here.

Now I am not a big fan of db.cf.x for both the reason @DaniFoldi mentioned, but also because it would make some of those "prepared" queries completely useless if you run that over both KV and DO spans, because of the nature of the systems are so incredibly different.

But I also love to follow some of those conventions so that we can make it easier for external parties to automatically include those in their queries.

So @plantfansam, would it be better to do say db.cf.kv.key or should we do a db.cf_kv.key? Or would we expect those 3rd parties to also filter based on the dbSystem key?

And we have had some discussions about the HTTP Span Name already, but I am relatively reluctant to go with the Semantic Convention there. We have no way of knowing a route upfront. The pathname is very likely to include things like IDs, so I really wouldn't want to treat that as the route. And without route, the span name would basically be GET or POST.

Now what we could do is at the end of the span check if the user has manually set the ROUTE information and then change the span name accordingly? But I am looking forward to people's opinion on that.

plantfansam commented 1 year ago

So @plantfansam, would it be better to do say db.cf.kv.key or should we do a db.cf_kv.key? Or would we expect those 3rd parties to also filter based on the dbSystem key?

I like db.cf.kv.key because it hews more closely to current semantic conventions. As an example, take the semconv attributes for aws dynamodb: aws.dynamodb.consumed_capacity, aws.dynamodb.table_names.

plantfansam commented 1 year ago

And we have had some discussions about the HTTP Span Name already, but I am relatively reluctant to go with the Semantic Convention there. We have no way of knowing a route upfront.

I wonder if we performantly allow users to configure this. So on boot you pass a config option like:

{
  "restfulAction/:id": {"regex": RegExp.new("https://foobar.com/restfulAction/[d+]"), // or whatever the regex is
  "anotherRestfulAction/:id": {"regex": RegExp.new("https://foobar.com/anotherRestfulAction/[d+]")
}

If config is unset, then default to HTTP span name as host, if config object set, try to parse. Would want to benchmark this for sure...

rdooley commented 1 year ago

Rather than muddy things with intermediate work, I'll wait until we have some stronger consensus on attribute names before updating this PR.

rdooley commented 1 year ago

So @plantfansam, would it be better to do say db.cf.kv.key or should we do a db.cf_kv.key? Or would we expect those 3rd parties to also filter based on the dbSystem key?

I like db.cf.kv.key because it hews more closely to current semantic conventions. As an example, take the semconv attributes for aws dynamodb: aws.dynamodb.consumed_capacity, aws.dynamodb.table_names.

Following that pattern we should end up with cf.kv.key and db.system="cloudflarekv" right?

evanderkoogh commented 1 year ago

And we have had some discussions about the HTTP Span Name already, but I am relatively reluctant to go with the Semantic Convention there. We have no way of knowing a route upfront.

I wonder if we performantly allow users to configure this. So on boot you pass a config option like:
{
  "restfulAction/:id": {"regex": RegExp.new("https://foobar.com/restfulAction/[d+]"), // or whatever the regex is
  "anotherRestfulAction/:id": {"regex": RegExp.new("https://foobar.com/anotherRestfulAction/[d+]")
}
If config is unset, then default to HTTP span name as host, if config object set, try to parse. Would want to benchmark this for sure...

I see where you are going, but that means that you need to perfectly duplicate your URL parsing. Once in your otel config and once in your router. And they could use different configuration mechanics.

And if you screw that up it is going to be hard to figure that out because the thing you are relying on to find that issue is wrong.

evanderkoogh commented 1 year ago

So @plantfansam, would it be better to do say db.cf.kv.key or should we do a db.cf_kv.key? Or would we expect those 3rd parties to also filter based on the dbSystem key?

I like db.cf.kv.key because it hews more closely to current semantic conventions. As an example, take the semconv attributes for aws dynamodb: aws.dynamodb.consumed_capacity, aws.dynamodb.table_names.

So AWS doesn’t use the ‘db’ prefix for their databases?

My question more is, do we have to stick with two dots or can we have more?

I prefer db.cf.kv.key, but if vendors are going to not parse that properly, but handle db.cf_kv.key properly, I would pick that.

rdooley commented 1 year ago

So AWS doesn’t use the ‘db’ prefix for their databases?

AWS doesn't (for some reason) but basically every other one with unique attribute keys (cassandradb used as example) does

My question more is, do we have to stick with two dots or can we have more?

I prefer db.cf.kv.key, but if vendors are going to not parse that properly, but handle db.cf_kv.key properly, I would pick that.

Making some assumptions based on our own experiences and a reading of the spec, but attribute keys are just strings so we should be fine to go with db.cf.kv.key

evanderkoogh commented 1 year ago

Cool.. I am happy to go with db.cf.kv.key and db.cf.do.key etc..

How about we revert the span name for now and I'll open up an issue to further discuss that. And then we can get all the db.* one in. Want to get those in as early as possible for people to realise that they have changed before we get to a proper 1.0.0

plantfansam commented 1 year ago

So AWS doesn’t use the ‘db’ prefix for their databases?

Looks like no, but I think we can 😄

My question more is, do we have to stick with two dots or can we have more?

I think it's OK to have more than 2 dots. See e.g. rabbitmq's messaging.rabbitmq.destination.routing_key

And if you screw that up it is going to be hard to figure that out because the thing you are relying on to find that issue is wrong.

Yeah, I think you're right. Needs more 🤔

How about we revert the span name for now and I'll open up an issue to further discuss that. And then we can get all the db.* one in. Want to get those in as early as possible for people to realise that they have changed before we get to a proper 1.0.0

I'm happy with that 😄

plantfansam commented 1 year ago

So @plantfansam, would it be better to do say db.cf.kv.key or should we do a db.cf_kv.key? Or would we expect those 3rd parties to also filter based on the dbSystem key?

I like db.cf.kv.key because it hews more closely to current semantic conventions. As an example, take the semconv attributes for aws dynamodb: aws.dynamodb.consumed_capacity, aws.dynamodb.table_names.

Following that pattern we should end up with cf.kv.key and db.system="cloudflarekv" right?

Yeah, I don't think there's an established convention for whether you should use e.g. db. or messaging. before vendor-specific attributes. Rabbitmq uses messaging. while dynamodb (as you point out) uses aws.. My personal preference is to include db. in the attr name (so db.cf.kv.key), but I don't think the community has an established perspective there.

rdooley commented 1 year ago

Just documenting the HTTP_ROUTE and span name behavior from upstream

they pull the route off the rpcMetadata here
and set the span name based on this (if defined) here
and here is a route they set during the test which is checked here

Gonna revert the route/span name stuff for now in the interest of having a cleaner PR here, but just wanted to document this somewhere with some links for a convenient reference point.

evanderkoogh commented 1 year ago

I put your comment in a HTTP span name issue.

evanderkoogh commented 1 year ago

Hey @rdooley, that last commit means the PR is ready to go right? So shouldn’t be a draft anymore?

Also, please don’t take the discussion to mean I don’t appreciate the PR. I absolutely love other people getting involved and improving things for not just themselves, but the entire community.

rdooley commented 1 year ago

Hey @rdooley, that last commit means the PR is ready to go right? So shouldn’t be a draft anymore?

I think so, but let me do a quick search of our span storage to ensure we don't have any other camelCase attrs I'm missing or anything that looks funky.

Also, please don’t take the discussion to mean I don’t appreciate the PR. I absolutely love other people getting involved and improving things for not just themselves, but the entire community.

This was perfect for me, discussion is good.

rdooley commented 1 year ago

There are a few more http attributes that I missed existing conventions of, and additionally for consistency I think the cf.* attributes added via gatherOutgoingCfAttributes should be cf.snake_case instead of cf.camelCase for consistency.

I'll do both of these additions as separate commits and then mark this PR as ready for review

rdooley commented 1 year ago

This branch is now (finally) ready for review

A few notes here about opentelemetry-js SemanticConventions we are using that differ from spec (I'll open this as an issue here and try to update opentelemetry-js I guess)

http.request_content_length should be http.request.body.size
http.status_code should be http.response.status_code
http.scheme should be url.scheme
http.user_agent should be user-agent.original

evanderkoogh commented 1 year ago

Thanks @rdooley! I am merging this now, but might be a day or two before I have a chance to release it. Currently on PTO

rdooley commented 1 year ago

Thanks @rdooley! I am merging this now, but might be a day or two before I have a chance to release it. Currently on PTO

Awesome, no rush on my end. I'm gonna try to get a look at #61 later this week and maybe get the ball rolling on #62 if i'm feeling ambitious

evanderkoogh / otel-cf-workers

Semantic conventions #53