Skip to content
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

[connector/servicegraph] New labels for service disambiguation and identification #31889

Closed
t00mas opened this issue Mar 21, 2024 · 6 comments
Closed
Labels
connector/servicegraph enhancement New feature or request needs triage New item requiring triage

Comments

@t00mas
Copy link
Contributor

t00mas commented Mar 21, 2024

Component(s)

connector/servicegraph

Is your feature request related to a problem? Please describe.

The connection_type label currently allows database, messaging_system, and virtual_node values.

For virtual_node values, there’s no clear distinction between which of the nodes is the virtual_node (e.g. client vs server), and has to be inferred from other attributes.

There’s also the case where just having a value of database or messaging_system falls short to identify if it’s also un-instrumented / virtual_node.

Describe the solution you'd like

  1. In the cases where connection_type had the value of virtual_node, generate a new label virtual_node with a value of either client or server.

2.1. In the cases where connection_type is of value database, surface either client_db_system or server_db_system labels, with the same value as the db.system resource attribute.

2.2. In the cases where connection_type is of value messaging_system, then surface either client_messaging_system or server_messaging_system labels, with the same value as the messaging.system resource attribute.

Describe alternatives you've considered

No response

Additional context

No response

@t00mas t00mas added enhancement New feature or request needs triage New item requiring triage labels Mar 21, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jpkrohling
Copy link
Member

I understand this wouldn't be a breaking change, right? All the changes are about adding new labels when specific values are specified.

@t00mas
Copy link
Contributor Author

t00mas commented Apr 24, 2024

Absolutely, the idea is for this to be fully optional, and about just adding a new label if enabled.

Also as an update to the issue, I think the change would only affect adding 1 new label, virtual_node, since the other 4 are covered under a combination of existing features (dimensions and prefixes).

jpkrohling added a commit that referenced this issue Jun 20, 2024
…n extra label for virtual node identification (#32826)

**Description:** 

- A new configuration option `enable_virtual_node_label` is added. When
enabled, it will add an extra label to the metrics, `virtual_node`,
indicating if the `client` or the `server` was the missing span.

**Link to tracking Issue:**
[31889](#31889)

**Testing:**

- Adding tests with
`github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden`.
- Testing the addition of `virtual_node` labels to the metrics, as well
as `connection_type: virtual_node` (as I think it was untested before).
- Also testing extra dimensions regarding client/server attributes.

**Documentation:**

- Added the new configuration option to the README

**Note to reviewers**

I have a suspicion that `connection_type: virtual_node` was broken.
There was a race condition on `store.Expire` so the expired edges were
completing after the metrics were built. I have added a
`p.store.Expire()` just at the start of `flushMetrics` to allow these
edges to contribute to the metrics. Please advise if you see something
wrong with this approach.

---------

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jpkrohling
Copy link
Member

jpkrohling commented Jul 4, 2024

@t00mas, was this handled by #32826? I'm assuming it was and I'm closing this issue, but let me know if there's still something pending and I'll reopen.

@t00mas
Copy link
Contributor Author

t00mas commented Jul 4, 2024

@t00mas, was this handled by #32826? I'm assuming it was and I'm closing this issue, but let me know if there's still something pending and I'll reopen.

That's right, I don't think anything is pending. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector/servicegraph enhancement New feature or request needs triage New item requiring triage
Projects
None yet
Development

No branches or pull requests

2 participants