Rename syslog-ident from redis to valkey.

[karthyuom]

This PR addresses the issue https://github.com/valkey-io/valkey/issues/301 in order to rename the syslog-ident from redis to valkey. Since this is a breaking change, it can be mentioned in the release note as well.

@hwware @zuiderkwast @PingXie @madolson

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 68.43%. Comparing base (19c4c64) to head (7c1a9ac). Report is 3 commits behind head on unstable.

:exclamation: Current head 7c1a9ac differs from pull request most recent head 312a69e. Consider uploading reports for the commit 312a69e to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #390      +/-   ##
============================================
+ Coverage     68.35%   68.43%   +0.07%     
============================================
  Files           108      108              
  Lines         61563    61569       +6     
============================================
+ Hits          42084    42134      +50     
+ Misses        19479    19435      -44     

Files	Coverage Δ
src/config.c	77.63% <100.00%> (+0.08%)	:arrow_up:

... and 14 files with indirect coverage changes

[PingXie]

I am aligned with this change. The only question I have is whether we should gate it behind the compat switch introduced in #306. @zuiderkwast wdyt?

[McFacePunch]

should gate it behind the compat switch

@PingXie Yes please do, without it being behind a flag it could break the drop-in nature of deploying valkey for some. There is a substantial possibility for impact of downstream systems that ingest the logs for further processing as the Identity is typically the first in a series of filters or pivots.

This would help ensure it remains a drop-in replacement and not a migration.

[PingXie]

Make sense. Thanks for the feedback @McFacePunch

[karthyuom]

Thanks @PingXie @McFacePunch for the comments. Please see the latest commit. I have no idea why that test-sanitizer fails.

[karthyuom]

I think we need some tests. The defaults can be tested in the existing test for the compat config. Custom value may need a new test case.

Sure, I will add tests to cover this new change.

Note that syslog-ident is an immutable config while the redis-compat is a mutable config. Changing it in runtime will not affect the syslog-ident. Is that expected or confusing?

is there any reason why extended-redis-compatibility config added as a mutable one?

• In my opinion, since this extended compatibility switch is a temporary config, we can change it as a immutable one (i.e., we shouldn't allow user to change at runtime). Changing syslog-ident as a mutable one may not be appropriate
• Otherwise, we don't need to worry much because the config extended-redis-compatibility will eventually have no effect at 9.0 and be removed at 10.0.

[zuiderkwast]

Is there any reason why extended-redis-compatibility config added as a mutable one?

I didn't think about mutable or immutable for this config before. :)

I think mutable can be useful. If users find that there is some compatibility problem with a client or another tool, they can enable it in runtime to fix the problem.

We can do like this: extended-redis-compatibility affects the default value for syslog-ident only at startup. If extended-redis-compatibility is changed in runtime, syslog-ident will not be affected. I think it's good behaviour. Actually, all default values are only used at startup.

[hwware]

@karthyuom When you compile valkey, try to run make SANITIZER=address, and run the test in your local to see if there are some memory leak. It is better run on Ubuntu 22, because Github CI run this on Ubuntu 22. Thanks