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.

Sign up for GitHub

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

Jump to bottom

servicegraph connector: Add metrics_flush_interval #27679

Closed
gouthamve opened this issue Oct 16, 2023 · 5 comments
Closed

servicegraph connector: Add metrics_flush_interval #27679

gouthamve opened this issue Oct 16, 2023 · 5 comments
Labels
connector/servicegraph enhancement New feature or request processor/servicegraph Service graph processor

Comments

@gouthamve
Copy link
Member

Component(s)

connector/servicegraph

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

spanmetrics connector has a metrics_flush_interval that controls how frequently metrics are pushed. Service graph connector should have the same.

Describe the solution you'd like

Add metrics_flush_interval to service graph connector.

Describe alternatives you've considered

No response

Additional context

No response
@gouthamve gouthamve added enhancement New feature or request needs triage New item requiring triage labels Oct 16, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

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

@gouthamve gouthamve changed the title servicegraph connector: Add metrics_flush_interval to servicegraph connector: Add metrics_flush_interval Oct 16, 2023
@crobert-1
Copy link
Member

Sounds like a valid feature request to me.

However, I looked into the implementation side of things and it looks like the servicegraph connector is really just a wrapper of the servicegraph processor, which is actually now deprecated. This means all of the logic being used is the processor's functionality.

It would be a pretty involved change since these two components are so heavily intertwined. I'll defer to code owners to decide what the best path forward is here.

@crobert-1 crobert-1 added processor/servicegraph Service graph processor and removed needs triage New item requiring triage labels Oct 16, 2023
@github-actions
Copy link
Contributor

Pinging code owners for processor/servicegraph: @jpkrohling @mapno. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot mentioned this issue Oct 17, 2023
Closed
@mapno mapno mentioned this issue Oct 20, 2023
Merged
jpkrohling added a commit that referenced this issue Oct 24, 2023
@mapno @jpkrohling
[servicegraphconnector] Add flush interval config (#27879)
3a27029 
**Description:** Add a config option to periodically flush metrics,
instead of flushing on every push.

**Link to tracking Issue:** <Issue number if applicable> #27679

**Testing:** <Describe what testing was performed and which tests were
added.> Added tests that verify metrics are flushed asynchronously

**Documentation:** <Describe the documentation added.> Documentation
added to `config.go`

---------

Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>
sigilioso pushed a commit to carlossscastro/opentelemetry-collector-contrib that referenced this issue Oct 27, 2023
@mapno @jpkrohling @sigilioso
[servicegraphconnector] Add flush interval config (open-telemetry#27879)
5ca3e6f 
**Description:** Add a config option to periodically flush metrics,
instead of flushing on every push.

**Link to tracking Issue:** <Issue number if applicable> open-telemetry#27679

**Testing:** <Describe what testing was performed and which tests were
added.> Added tests that verify metrics are flushed asynchronously

**Documentation:** <Describe the documentation added.> Documentation
added to `config.go`

---------

Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
@mapno @jpkrohling @jmsnll
[servicegraphconnector] Add flush interval config (open-telemetry#27879)
9a0a136 
**Description:** Add a config option to periodically flush metrics,
instead of flushing on every push.

**Link to tracking Issue:** <Issue number if applicable> open-telemetry#27679

**Testing:** <Describe what testing was performed and which tests were
added.> Added tests that verify metrics are flushed asynchronously

**Documentation:** <Describe the documentation added.> Documentation
added to `config.go`

---------

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

However, I looked into the implementation side of things and it looks like the servicegraph connector is really just a wrapper of the servicegraph processor, which is actually now deprecated. This means all of the logic being used is the processor's functionality.

We really should have it the other way around, with the connector being the reference implementation, and the processor wrapping it.

@ptodev
Copy link
Contributor

ptodev commented Dec 15, 2023

@gouthamve @mapno Can this issue be closed now, given that #27879 was merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
connector/servicegraph enhancement New feature or request processor/servicegraph Service graph processor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jpkrohling @ptodev @gouthamve @crobert-1