fix: make helm use grpc for compactor address

[rurod]

What this PR does / why we need it:
Align the computation logic of the compactor address with how other address are computed.
Moreover, it makes use of grpc, as according to the Loki codebase, it's the preferred way :
https://github.com/grafana/loki/blob/46b2271aacd26734979667da61d5c9d24a8efb2b/pkg/loki/modules.go#L1256C1-L1274C2

Which issue(s) this PR fixes:
Kind of fixes #17100, as using the full hostname of the service rather than the short name helps comply with the no_proxy instruction, even if not mandatory

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[jkroepke]

Hi @rurod I looked into the code where loki.compactorAddress is used.

It is used in

loki/production/helm/loki/values.yaml

Lines 354 to 357 in 01065da

	commonConfig:
	path_prefix: /var/loki
	replication_factor: 3
	compactor_address: '{{ include "loki.compactorAddress" . }}'

and according docs, compactor_address requires and address in http://address:port format. (https://grafana.com/docs/loki/latest/configure/#common_config). In any case, define gRPC port here should not work.

[rurod]

Hi @jkroepke, thanks for having a look at my PR and catching this.

What do you think about replacing the compactor_address parameter by the compactor_grpc_address one in the configuration template?

Is there any specific reason to use http for the compacto rather than grpc ?

[jkroepke]

Is there any specific reason to use http for the compacto rather than grpc ?

Looking through the git history, I cant find any valid reason.

However, I found #7804 which advertise gRPC over HTTP.

Generally, I would say that the Helm chart should prefer gRPC as well.
However, this would result in a theoretical breaking change, because any user-defined value for compactor_address would then be ignored.
If both are defined, Loki prioritizes the compactor_grpc_address value.

But I feel that the theoretical breaking change is an edge which can be covered by an fair warning in https://github.com/grafana/loki/blob/main/production/helm/loki/CHANGELOG.md

[jkroepke]

hi @rurod

the PR will be discussed internally next week.

[rurod]

Hi @jkroepke, thanks for letting me know.

In the meantime, I updated the PR with your suggestions.

By the way, to avoid the breaking change, there is another implementation that could work :

1. Creating a new Helm named template called loki.compactorGrpcAddress
2. Letting the default commonConfig use http and the already existing loki.compactorAddress

This way, administrators would have the option to switch to grpc when it best suits them and Helm chart maintainers could switch to grpc as default config when they'd like.

Let me know what you think and the output of your internal meeting, thanks.

[jkroepke]

Hi @rurod

I got feedback from the internal team. The breaking change is fine in that context, because if someone customize the compacter address, it's an edge-case scenario.

You suggestion is fine as well, but I would like to keep it simple for now.

[jkroepke]

@rurod could you please take note of my comments and resolve conflicts?

[rurod]

Hi @jkroepke, couldn't work on this earlier.
I will make the changes and update the PR.

[jkroepke]

Hi, thanks for your changes. I looks good to me. Would you resolve the conflicts? And I guess, since we are changing the values, make helm-docs must be executed.

[rurod]

Done, thanks a lot for taking the time to guide me during this PR!

[jkroepke]

LGTM - just one nit.

Done, thanks a lot for taking the time to guide me during this PR!

No problem, always welcome!

[JStickler]

@rurod could you please re-generate the Helm Chart reference by running 'make -C docs sources/setup/install/helm/reference.md'.

[rurod]

Hi @JStickler, done

[Jayclifford345]

LGTM -> @JStickler I will create a PR to document the potential breaking change.