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

Fix naming convention issue in helm chart #660

Merged
merged 9 commits into from
Feb 23, 2023

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Feb 9, 2023

While setting renameFieldsSck to true, the resource/logs sets the fields as per the SCK convention and copies their values from otel fields. But there are some otel fields which are not set in the first place.
So, move the resource above resource/logs as it sets all the required fields and the latter can then copy without any issues.
Also, add label_app field, if renameFieldsSck is true

The SCK's field container_image is split into two parts as per Otel convention i.e. image_name and image_tag, so combine them if user has enabled renameFieldsSck

@VihasMakwana VihasMakwana requested review from a team as code owners February 9, 2023 12:25
@VihasMakwana VihasMakwana marked this pull request as draft February 9, 2023 13:02
@VihasMakwana VihasMakwana force-pushed the fix-naming-convention-issue-sck branch from aeaaecf to c3d4f49 Compare February 9, 2023 13:37
@VihasMakwana VihasMakwana force-pushed the fix-naming-convention-issue-sck branch from c3d4f49 to 8aaa664 Compare February 9, 2023 13:42
@dmitryax
Copy link
Contributor

dmitryax commented Feb 9, 2023

This solves the problem only for a specific label fetched from the pods, fetching of that label appeared to be set by default configuration, but it might change in the future. Instead of hardcoding the rules, maybe we can just ask users that want this label in SCK format to add additional configuration which would be?

  fromLabels:
    - key: app
      tag_name: label_app

@VihasMakwana
Copy link
Contributor Author

This solves the problem only for a specific label fetched from the pods, fetching of that label appeared to be set by default configuration, but it might change in the future. Instead of hardcoding the rules, maybe we can just ask users that want this label in SCK format to add additional configuration which would be?

  fromLabels:
    - key: app
      tag_name: label_app

Yes, we can ask users but the problem is that the SCK-Otel wouldn't provide all the SCK labels as mentioned in https://github.com/signalfx/splunk-otel-collector-chart/blob/main/docs/migration-from-sck.md#extracted-fields-for-logs

There are a couple of fields missing, so I guess it's better to add those fields in our processor with existing fields.

@VihasMakwana VihasMakwana marked this pull request as ready for review February 10, 2023 07:14
@dmitryax
Copy link
Contributor

Ok. Feel free to keep it, but please make sure it's only applicable if Values.splunkPlatform.fieldNameConvention.keepOtelConvention is set to true. container_image should not be added by default

@VihasMakwana VihasMakwana force-pushed the fix-naming-convention-issue-sck branch from 4c81654 to e9f1403 Compare February 10, 2023 08:54
@VihasMakwana
Copy link
Contributor Author

Ok. Feel free to keep it, but please make sure it's only applicable if Values.splunkPlatform.fieldNameConvention.keepOtelConvention is set to true. container_image should not be added by default

Took care of that.

log_statements:
- context: log
statements:
{{- if .Values.splunkPlatform.fieldNameConvention.renameFieldsSck }}
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to apply the condition to the whole receiver and its reverence in the pipeline

Copy link
Contributor Author

@VihasMakwana VihasMakwana Feb 15, 2023

Choose a reason for hiding this comment

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

I agree, but the reason why I added it here is that if someone wants to add anything to the transform processor in the future, it would be easier to do.
I have also made transform/logs in a similar fashion to resource/logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

If users want to add another processor and configure it, it's easy to define in the agent.config section. We should not add components with an empty body just to make them visible, it's a lot of different components available

Copy link
Contributor Author

@VihasMakwana VihasMakwana Feb 17, 2023

Choose a reason for hiding this comment

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

Yes, they can, but many customers will be migrating from SCK to SCK-Otel as SCK will be deprecated soon [End of Support].
That's why I'm adding this new processor. The only reason for adding a new processor is that to make SCK-Otel backward compatible with SCK. For other stuff, users can make use of agent.config as you suggested.

Copy link
Contributor

@dmitryax dmitryax Feb 21, 2023

Choose a reason for hiding this comment

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

Please don't add a new processor in the production config if it's not being used by default, it's confusing. Just mention how to configure it in the migration doc

@VihasMakwana VihasMakwana force-pushed the fix-naming-convention-issue-sck branch from 243da9a to 30e649b Compare February 15, 2023 09:29
@VihasMakwana VihasMakwana force-pushed the fix-naming-convention-issue-sck branch from 30e649b to 2001c2d Compare February 21, 2023 10:19
@dmitryax
Copy link
Contributor

@vihas-splunk please run make render

@VihasMakwana
Copy link
Contributor Author

@vihas-splunk please run make render

Done, can you approve the workflow? I can go ahead and merge after that
I've verified that pre-commit checks are passing on my local machine.

@VihasMakwana VihasMakwana force-pushed the fix-naming-convention-issue-sck branch from df75496 to 2001c2d Compare February 23, 2023 06:20
@VihasMakwana
Copy link
Contributor Author

@dmitryax PR looks good to merge

@dmitryax dmitryax merged commit 855add6 into signalfx:main Feb 23, 2023
dmitryax added a commit to dmitryax/splunk-otel-collector-chart that referenced this pull request May 4, 2023
Following the merge of signalfx#660, the `resource` processor in the logs pipeline was placed before the `resourcedetection` processor. In contrast, the other pipelines have the `resource` processor after the `resourcedetection` processor. This order is crucial in GCP because the `resourcedetection` processor's `gcp` detector sets the `k8s.cluster.name` attribute, which we then override with the user-provided value in the `resource` processor. By keeping the `resource` processor after the `resourcedetection` processor, we ensure that any `k8s.cluster.name` attribute is always set with the user-provided value.

This change was implemented to ensure that the order of processors is consistent across all pipelines, guaranteeing that the `k8s.cluster.name` attribute is always set to the value provided in the `clusterName` field of the values.yaml file.
dmitryax added a commit to dmitryax/splunk-otel-collector-chart that referenced this pull request May 4, 2023
Following the merge of signalfx#660, the `resource` processor in the logs pipeline was placed before the `resourcedetection` processor. In contrast, the other pipelines have the `resource` processor after the `resourcedetection` processor. This order is crucial in GCP because the `resourcedetection` processor's `gcp` detector sets the `k8s.cluster.name` attribute, which we then override with the user-provided value in the `resource` processor. By keeping the `resource` processor after the `resourcedetection` processor, we ensure that any `k8s.cluster.name` attribute is always set with the user-provided value.

This change was implemented to ensure that the order of processors is consistent across all pipelines, guaranteeing that the `k8s.cluster.name` attribute is always set to the value provided in the `clusterName` field of the values.yaml file.
dmitryax added a commit to dmitryax/splunk-otel-collector-chart that referenced this pull request May 4, 2023
Following the merge of signalfx#660, the `resource` processor in the logs pipeline was placed before the `resourcedetection` processor. In contrast, the other pipelines have the `resource` processor after the `resourcedetection` processor. This order is crucial in GCP because the `resourcedetection` processor's `gcp` detector sets the `k8s.cluster.name` attribute, which we then override with the user-provided value in the `resource` processor. By keeping the `resource` processor after the `resourcedetection` processor, we ensure that any `k8s.cluster.name` attribute is always set with the user-provided value.

This change was implemented to ensure that the order of processors is consistent across all pipelines, guaranteeing that the `k8s.cluster.name` attribute is always set to the value provided in the `clusterName` field of the values.yaml file.
dmitryax added a commit that referenced this pull request May 4, 2023
Following the merge of #660, the `resource` processor in the logs pipeline was placed before the `resourcedetection` processor. In contrast, the other pipelines have the `resource` processor after the `resourcedetection` processor. This order is crucial in GCP because the `resourcedetection` processor's `gcp` detector sets the `k8s.cluster.name` attribute, which we then override with the user-provided value in the `resource` processor. By keeping the `resource` processor after the `resourcedetection` processor, we ensure that any `k8s.cluster.name` attribute is always set with the user-provided value.

This change was implemented to ensure that the order of processors is consistent across all pipelines, guaranteeing that the `k8s.cluster.name` attribute is always set to the value provided in the `clusterName` field of the values.yaml file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants