[exporter/coralogix] Create dynamic subsystem name #7957
Conversation
exporter/coralogixexporter/client.go
Outdated
| ApplicationName: c.cfg.AppName, | ||
| SubsystemName: subsystem, | ||
| } | ||
| c.cfg.GRPCClientSettings.Headers["subsystemName"] = subsystem |
There was a problem hiding this comment.
If your user has set this header themselves in the config, you are overriding it without warning. Be nice to your users and inform them that this is happening.
There was a problem hiding this comment.
A warning message is added
jpkrohling
changed the title
|
Hi @jpkrohling any update on the PR? |
|
searching for an issue with problems, freezing for now |
|
Hi @jpkrohling any updates? |
|
See my comments about the config breaking compatibility with the previous version. |
|
Hi @jpkrohling, according to your comment about breaking changes. I add the changes to the changelog file. In addition, I update the readme, tests and config_test file. The 'subsystems' config option is not available anymore. |
|
@ofirshmuel, please go over the document I shared earlier: https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#breaking-changes The document above has concrete steps you should be taking to prevent breaking your users without notice. As it is, just removing the config option isn't acceptable without deprecating it first. |
|
@jpkrohling I mark the |
exporter/coralogixexporter/client.go
Outdated
| Metadata: &cxpb.Metadata{ApplicationName: c.cfg.AppName, SubsystemName: batch.GetProcess().GetServiceName()}, | ||
| }, grpc.WaitForReady(c.cfg.WaitForReady)) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to push trace data via Coralogix exporter %w", err) |
There was a problem hiding this comment.
Please, follow the Go conventions by not uppercasing the first letter from the error message
There was a problem hiding this comment.
I changed the uppercase to lowercase according to the conventions.
exporter/coralogixexporter/client.go
Outdated
| } | ||
| c.logger.Debug("Trace was sent successfully") | ||
| } else { | ||
| return fmt.Errorf("Failed to push trace data via Coralogix exporter because batch.process.serviceName is empty") |
There was a problem hiding this comment.
Same here and possibly elsewhere.
There was a problem hiding this comment.
I changed the uppercase to lowercase according to the conventions.
exporter/coralogixexporter/config.go
Outdated
| @@ -56,7 +58,7 @@ func (c *Config) Validate() error { | |||
| return fmt.Errorf("`appName` not specified, please fix the configuration file") | |||
| } | |||
| if c.SubSystem == "" { | |||
| return fmt.Errorf("`subSystem` not specified, please fix the configuration file") | |||
| return fmt.Errorf("`subSystem` not specified, please fix the configuration file.\nPay attention: deprecated subsystem will be removed in the next version") | |||
There was a problem hiding this comment.
Imagine error messages in a broader context: they bubble up and are printed to the user by the component that is the closest to the user, collecting all the breadcrumbs until it gets there. In that context, new lines aren't desirable, as your message would end up looking like this:
2022-03-11 14:10:12.1234 ERR could not start blurbs: failed to parse config file: `subSystem` not specified, please fix the configuration file
Pay attention: deprecated subsystem will be removed in the next version.
That said, you should really make room for your users to migrate to the replacement already. If they won't be allowed to specify this option, then make it possible for your users to not specify this already and leave their config files ready for the next version.
There was a problem hiding this comment.
thanks @jpkrohling , I fixed the error message and let the users add or remove 'subsystem' option from the config file in this version.
Description:
Create dynamic subsystem name for Coralogix endpoint.
The subsystem name is taken from the service_name of span.
Add exporter timeout option.
Link to tracking Issue:
Testing:
Documentation: