Skip to content

[connector/spanmetrics] Replace connector.servicegraph.virtualNode feature gate with config options#33282

Closed
ptodev wants to merge 2 commits intoopen-telemetry:mainfrom
ptodev:update-servicegraph
Closed

[connector/spanmetrics] Replace connector.servicegraph.virtualNode feature gate with config options#33282
ptodev wants to merge 2 commits intoopen-telemetry:mainfrom
ptodev:update-servicegraph

Conversation

@ptodev
Copy link
Contributor

@ptodev ptodev commented May 29, 2024

Description:
This PR intends to deprecate the connector.servicegraph.virtualNode feature gate and the virtual_node_peer_attributes config argument. They will be replaced by new client_virtual_nodes and server_virtual_nodes config arguments.

The connector.servicegraph.virtualNode feature gate has a few issues:

  • It's not possible to configure server and client virtual nodes independently. We can't disable one but enable the other. For example, the only ways to disable client virtual nodes are:
    • Disabling the feature gate entirely. This is not a long term solution, because feature gates are meant to be only temporary.
    • Set virtual_node_peer_attributes to an empty list. This is weird, because virtual_node_peer_attributes is only meant to be used for server virtual nodes - not for client virtual nodes.
  • The client virtual nodes are always set to "user". It's not possible to extract them from span attributes.

Testing:
This PR adds tests which were added in #17350 removed in #30149.
I am also happy to add more tests if the overall goal of the PR is accepted.

Documentation:
I will update the docs once the overall goal of the PR has been accepted.

cc @JaredTan95 and @ogxd who worked on virtual nodes previously.

@JaredTan95
Copy link
Member

@ptodev thx for your contributions, and can you explain a little more about why we need to set up a virtual node for the client and server separately?

@ptodev
Copy link
Contributor Author

ptodev commented May 29, 2024

why we need to set up a virtual node for the client and server separately?

Hi @JaredTan95, thank you for the quick reply! I personally do not need this feature, but I think it may be useful for a few reasons:

  • Some users may want either client or server virtual nodes, but not both.
  • Even if the users want both client and server virtual nodes, they may want them configured differently.
  • Using the same VirtualNodes struct for both client and server nodes means the config is easier to extend over time with new config attributes.

This feels relatively low effort to implement and I hope it solves enough use cases for most folks without being overengineered.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jun 13, 2024
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants