elastic / elastic/apm

Current span naming status as a step towards name alignment

Open
#446 17 comments 0 reactions 0 assignees View on GitHub
apm-agents discussion
Dominant language
Gherkin
Stars
427
Forks
125
PR merge metrics
No merged PRs in 30d

Description

# Aligning span names

This is the initial step towards aligning span names across agents.
Naturally, it only deals with frameworks that are not language-specific. We have specs for those, but in some cases, these specs were added in retrospect, reflecting the naming-chaos at the time, so we may want to revisit.

@elastic/apm-agent-devs Please take the time to fill some examples in the tables and vote wherever there is a ![poll-topic-xxl](https://user-images.githubusercontent.com/41850454/120643994-7e130b00-c47f-11eb-94ba-826b9b483c2a.png) sign - either add 👍 to an existing option, or add a new one with your 👍 on it.
After I gather your feedback, I will make the spec PRs accordingly wherever required.

[OpenTelemetry general Span guidelines](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#span) for reference.

## HTTP

The naming scheme for transactions is not rigid in the existing [spec](https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-http.md#http-transactions). It is more so for spans, where names should have the format ` `.

[OpenTelemetry HTTP spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md#name) for reference.

Existing state:
| Agent | Transaction | Span |
|---|---|---|
| Java | `GET unknown route`
`ServletClassName#doGet`
`GET /testWithPathMethod/{id}` |`GET 127.0.0.1`
`GET [::1]`
`GET localhost` |
| .NET |
| Go | `GET unknown route`
`GET /hello/:name`
`GET /hello/{name}` | `GET host:port`
| Node.js | `GET unknown route` (vanilla HTTP server)
`GET /hello/:name` (typical with frameworks)
`static file` (if using express.static())
`CORS preflight` (CORS preflight req with hapi framework) | `POST localhost:9200/myIndex*/_search` (full URL usage is https://github.com/elastic/apm-agent-nodejs/issues/2067)
`GET http://localhost:52394/sub` (http2, same issue) |
| PHP | `GET /users/{id}` | `POST example.com` |
| Python | `GET /users/{id}`
(only for frameworks that expose the necessary routing info ) | `POST example.com` |
| Ruby | `PostsController#index` (most likely)
`GET /users/:id`
`POST /publish`
`Rack` (last resort) | `POST example.com`
`GET elastic.co` |

![poll-topic-xxl](https://user-images.githubusercontent.com/41850454/120643994-7e130b00-c47f-11eb-94ba-826b9b483c2a.png) Choose one of the followings:
- Spec is good enough as is 👍 👍 👍 👍 👍
- Spec should change (_replace with your suggestion and add a +1_)

## DB

The [spec](https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-db.md) defines a convention for each DB type.

[OpenTelemetry DB spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#semantic-conventions-for-database-client-calls) for reference.

Existing state:
| Agent | SQL DB | Redis | MongoDB | GraphQL | Elasticsearch | DynamoDB | S3 |
|---|---|---|---|---|---|---|---|
| Java | `SELECT FROM table` | `SET` | `db.collection.drop` | NA | `Elasticsearch: GET /index/_search` | NA | NA |
| .NET |
| Go | `SELECT FROM table` | `SET`
`(flush pipeline)` | `${collection}.${command}`
`aggregate` | NA | `Elasticsearch: GET /index/_search` | `DynamoDB GetItem Movies` | `S3 PutObject bucket`
| Node.js | `SELECT FROM table_name`
`UPDATE table_name` | `SET` | `db.collection.command` e.g. `elasticapm.test.insert`, `system.$cmd.ismaster` | Transaction name: `[opName] queryNames, ... (/path)`
Span name: `GraphQL: [opName] queryNames, ...`
(see [note](https://github.com/elastic/apm/issues/446#issuecomment-858137915) below) | `Elasticsearch: ${method} ${path}` e.g. `Elasticsearch: POST /myIndex*/_search` | [Not yet implemented](https://github.com/elastic/apm-agent-nodejs/issues/1953) | [Not yet implemented](https://github.com/elastic/apm-agent-nodejs/issues/1954) |
| PHP | `SELECT FROM table` | Not supported yet | Not supported yet | Not supported yet | Not supported yet | Not supported yet | Not supported yet |
| Python | `SELECT FROM table` | `SET`| `db.collection.drop` | NA| `ES GET /myindex/_search` | `DynamoDB Query tableName` | `S3 PutObject bucket` |
| Ruby | `SELECT FROM table` | `SET` | `db.collection.drop` | NA (outgoing) | `GET _search` | `DynamoDB Query tableName` | `S3 CreateBucket bucket-name` |

The spec includes prefixed names like:
- `DynamoDB UpdateItem my_table`
- `S3 GetObject my-bucket`
- `Elasticsearch: GET /index/_search`

![poll-topic-xxl](https://user-images.githubusercontent.com/41850454/120643994-7e130b00-c47f-11eb-94ba-826b9b483c2a.png) Choose one of the followings:
- Lose prefixes altogether: 👍 👍 👍
- Keep prefix, lose colon in all cases: (👍 [or render `subtype` in waterfall](https://github.com/elastic/apm/issues/446#issuecomment-858151934))
- Keep prefix with colon:

## Messaging

The [spec](https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-messaging.md) defines naming convention that contains lots of prefixing.
Kafka is not represented in the spec and in Java it is really bad (100% my fault 🤦‍♂️ ) - a combination of API-based for send spans and prefix-based for receive spans. This is because we don't really have an API in the Kafka client for a retrieval of one record, there is only API for reading a batch, which is not a good fit for transaction name that is based on a single record.

[OpenTelemetry messaging spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/messaging.md#span-name) for reference.

Existing state:
| Agent | Kafka | RabbitMQ | SQS | SNS | Other |
|---|---|---|---|---|---|
| Java | Send spans:
`KafkaProducer#send to MyTopic`
Transactions:
`Kafka record from MyTopic` | `RabbitMQ RECEIVE from MyQueue` | NA | NA | `JMS SEND to MyQueue` |
| .NET |
| Go | NA | NA | `SQS SEND to queue` | `SNS PUBLISH myTopic` | NA
| Node.js | Not supported | Not supported | `SQS SEND to queue_name`
`SQS RECEIVE from queue_name`
`SQS DELETE from queue_name` | Not supported | NA
| PHP | Not supported yet | Not supported yet | Not supported yet | Not supported yet | Not supported yet |
| Python |
| Ruby | NA | NA | `SQS SEND to my-queue` | `SNS PUBLISH to MyTopic` | NA

![poll-topic-xxl](https://user-images.githubusercontent.com/41850454/120643994-7e130b00-c47f-11eb-94ba-826b9b483c2a.png) Choose one of the followings:
- With prefix, e.g. `Kafka send to MyTopic`
- General form, human-readable, e.g. `Record send to MyTopic` or `Message received from MyQueue`
- General form, OTel scheme: ` `, e.g. `MyTopic send` or `MyQueue receive` 👍 👍
- API-based names, e.g. `MeassgeProducer#send` *
- Other (_replace with your suggestion and add a +1_)

\* Note: it is common in messaging clients that the receive API is designed for batches, in which case the API cannot be used as is for the transaction name. So if you choose this option, propose a name for such transactions.

## gRPC

The [spec](https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-grpc.md#grpc-support-in-agents) defines to used the method as the name, eg: `/helloworld.Greeter/SayHello`.
Since this is a fairly new spec that was approved lately, I am assuming that everybody is happy with it.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.