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/servicegraphconnector] Adds a new config option to enable messaging_system's metric generation #33881

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

t00mas
Copy link
Contributor

@t00mas t00mas commented Jul 3, 2024

Description:

  • A new configuration option enable_messaging_system_latency_histogram is added. When enabled, it will generate a new histogram metric for nodes of connection_type=messaging_system, traces_service_graph_request_messaging_system_seconds

Link to tracking Issue: #30856

Testing:

  • Adds a new unit test.

Documentation:

  • Updated README

@t00mas t00mas requested a review from jpkrohling as a code owner July 3, 2024 09:33
@t00mas t00mas requested a review from a team July 3, 2024 09:33
Copy link
Contributor

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but is this adding any new capabilities to the connector? It's simply adding a new metric with info already collected in other latency metrics. What problem is that solving?

@t00mas
Copy link
Contributor Author

t00mas commented Jul 15, 2024

Maybe I'm missing something, but is this adding any new capabilities to the connector? It's simply adding a new metric with info already collected in other latency metrics. What problem is that solving?

All that's true, it's just optionally enabling the creation of a new metric dedicated to messaging systems. It's adapted from grafana/tempo#3453 and was suggested here #30856

@mapno
Copy link
Contributor

mapno commented Jul 15, 2024

Maybe I'm missing something, but is this adding any new capabilities to the connector? It's simply adding a new metric with info already collected in other latency metrics. What problem is that solving?

All that's true, it's just optionally enabling the creation of a new metric dedicated to messaging systems. It's adapted from grafana/tempo#3453 and was suggested here #30856

But that Tempo PR introduces a new way of measuring latency in messaging systems

My proposal is as follows:

Add a new latency histogram metric.
The calculation will involve subtracting the producer end_time from the consumer start_time.

IIUC, this one does not.

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 Jul 30, 2024
@Frapschen
Copy link
Contributor

Can we implement this feature in the same way the current DB type span does?

@Frapschen Frapschen removed the Stale label Aug 7, 2024
@t00mas
Copy link
Contributor Author

t00mas commented Aug 12, 2024

Sorry I left this unattended for a while, I took a break. I'll address the missing features for this PR ASAP.

Thanks for the reviews and comments!

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 Aug 27, 2024
@JaredTan95
Copy link
Member

@t00mas Hi, any updates? just friendly ping, take your time.

@github-actions github-actions bot removed the Stale label Aug 28, 2024
@t00mas
Copy link
Contributor Author

t00mas commented Sep 2, 2024

@t00mas Hi, any updates? just friendly ping, take your time.

working on this this week, sorry for the delay

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 Stale and removed Stale labels Sep 17, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

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 Oct 3, 2024
@Frapschen Frapschen removed the Stale label Oct 8, 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.

5 participants