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

Source information is not available in signalfx #1606

Closed
zeldigas opened this issue Sep 26, 2019 · 13 comments
Closed

Source information is not available in signalfx #1606

zeldigas opened this issue Sep 26, 2019 · 13 comments
Labels
enhancement A general enhancement registry: signalfx A SignalFX Registry related issue

Comments

@zeldigas
Copy link

zeldigas commented Sep 26, 2019

Current reporter for SignalFx does not send information about "source", that is by default equal to hostname.

SFX team's dropwizard reporter sends this information as dedicated dimension, appended to every data point: https://github.com/signalfx/signalfx-java/blob/master/signalfx-codahale/src/main/java/com/signalfx/codahale/reporter/AggregateMetricSenderSessionWrapper.java#L195

Micrometer implementation sets source field in dataPoint, but looks like it does not work as expected, because there is no dimension with a value having this info.

Expected result:

  1. It is possible to enable behavior that all reported data point has additional dimension with value equal to source() property.
  2. It is also should be allowed to define tag name with sf_ prefix so that reported data can keep compatibility with already existing data (some legacy examples for signal fx used sf_source for this info)

I'm fine if this behavior would be optional and enabled by special config parameter, e.g. management.metrics.export.signalfx.reportSource

@zeldigas
Copy link
Author

I'm ready to send PR for this issue if you are fine to add this improvement

@shakuzen shakuzen added the registry: signalfx A SignalFX Registry related issue label Sep 29, 2019
@shakuzen
Copy link
Member

shakuzen commented Sep 29, 2019

Right now we're just passing the configured SignalFxConfig#source as the defaultSourceName parameter to the AggregateMetricSender provided by SignalFx's Java client. I'm wondering why that doesn't result in the expected behavior and if the change belongs in Micrometer or SignalFx's Java client.


Edit: here's the specific code referenced above:

AggregateMetricSender metricSender = new AggregateMetricSender(this.config.source(),
this.dataPointReceiverFactory, this.eventReceiverFactory,
new StaticAuthToken(this.config.accessToken()), this.onSendErrorHandlerCollection);

@zeldigas
Copy link
Author

Maybe some SFX folks could comment here. @mpetazzoni, @beccatortell, appreciate you can comment on the source field in protobuf spec compared to dimension

@zeldigas
Copy link
Author

zeldigas commented Oct 1, 2019

@shakuzen I did some search in SFX docs and while source field is present in protobuf descriptor, there is no such field in http api docs as well as other docs:

  1. https://developers.signalfx.com/ingest_data_reference.html - API endpoint definition to send metric data
  2. https://docs.signalfx.com/en/latest/getting-started/concepts/data-model.html - description of data model
  3. https://docs.signalfx.com/en/latest/metrics-metadata/metrics-metadata.html#metrics-metadata - various metadata about metrics only includes dimensions

@zeldigas
Copy link
Author

zeldigas commented Oct 7, 2019

@shakuzen do you have any comments? Are you waiting for some SignalFX rep to comment or we can count on some "de facto" behavior?

@shakuzen shakuzen added the enhancement A general enhancement label Oct 21, 2019
@shakuzen shakuzen added this to the 1.4.0 milestone Oct 21, 2019
@shakuzen
Copy link
Member

Sorry for the delay in response. I was taking some time off and then busy with a conference.

I think we can proceed with making the change to add the source dimension if it is not already set. I don't think it needs to be made configurable, since this seems to be the correct and expected behavior.

I would still be curious to know what is the purpose of setting the defaultSourceName on AggregateMetricSender if that info does not get sent to the server. However, we don't need to block progress on this for that.

@zeldigas Are you willing to send a pull request for this?

@zeldigas
Copy link
Author

Yes, I'll send PR this week

@zeldigas
Copy link
Author

PR is opened for this repo, but if I understand correctly I also need to do a follow up PR to spring-boot-actuator once this one is merged, right?

@shakuzen
Copy link
Member

if I understand correctly I also need to do a follow up PR to spring-boot-actuator once this one is merged, right?

If there are additional configuration options, then yes, there will need to be a change in Spring Boot to support those.

@shakuzen shakuzen modified the milestones: 1.4.0, 1.5.0 Mar 8, 2020
@shakuzen shakuzen modified the milestones: 1.5.0, 1.6.0 Apr 23, 2020
@shakuzen shakuzen modified the milestones: 1.6.0, 1.7.0 Oct 29, 2020
@shakuzen shakuzen modified the milestones: 1.7.0-M1, 1.7 backlog Mar 8, 2021
@shakuzen shakuzen modified the milestones: 1.7 backlog, 1.x May 12, 2021
@rajanigk
Copy link

rajanigk commented Aug 1, 2022

Is this issue fixed? Which version of the following library it is fixed? Also should i update springboot version for this? And what property to needs to be set to get to the filter on the source in splunk apm metrics
< dependency>
io.micrometer
micrometer-registry-signalfx
< /dependency>

@rajanigk
Copy link

rajanigk commented Aug 1, 2022

we used the splunk-otel-javaagent and are able to push metrics and filter it based on sf_service. When we push metrics using spring-boot and the micrometer-signalfx-registry library as per the springboot documentation at https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#actuator.metrics.export.signalfx, not able to filter the metrics based on the application. So thought the source dimension is for that filtering. I tried with spring-boot 2.7.2 which pulls the 1.9.2 version of micrometer-registry-signalfx the source is not coming to metric so that i could filter the metric based on it. If there is any other way to filter the metric to differentiate between different applications, please do let me know.

@izeye
Copy link
Contributor

izeye commented Jan 25, 2025

Now that the SignalFX support has been deprecated in #5807, this enhancement request issue seems to be able to be closed.

@shakuzen
Copy link
Member

Thanks @izeye. Closing due to #5807

@shakuzen shakuzen closed this as not planned Won't fix, can't repro, duplicate, stale Jan 27, 2025
@jonatan-ivanov jonatan-ivanov removed this from the 1.x milestone Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement registry: signalfx A SignalFX Registry related issue
Projects
None yet
Development

No branches or pull requests

5 participants