Change default syslog-ident from redis to valkey

[karthyuom]

Default value for the "syslog-ident" config changed from "redis" to "valkey".

Fixes #301.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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%)	⬆️

... 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.

[zuiderkwast]

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.

We should add a test with a user-configured value.

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?

[zuiderkwast]

This looks like it's overriding any value configured by the user.

[karthyuom]

Yes, you are right. It is little bit tricky to gate behind the redis-compat config than I thought before :)
Anyway, I will fix this problem.

[zuiderkwast]

An idea: Maybe we can set the default value at createStringConfig("syslog-ident", ...) to NULL. Then, here, we can set it to "redis" or SERVER_NAME if it still has no value. I didn't try. It's just an idea.

Suggested change

	server.syslog_ident = server.extended_redis_compat ? "redis" : SERVER_NAME;
	if (server.syslog_ident == NULL)
	server.syslog_ident = server.extended_redis_compat ? "redis" : SERVER_NAME;

[hwware]

I think adding server.extended_redis_compat condition here makes thing complicated.
In other places, by adding server.extended_redis_compat, the idea is to makes output log compatible with Redis version. However, considering the redis case here: the only values for syslog-ident are redis and other value; and for valkey case: the only values for syslog-ident are valkey and other value.

And we can also find the config syslog-ident is IMMUTABLE, thus I think we do not need to touch other files, only update the default value to "valkey" in config.c is enough.

If the client wants to use "syslog-ident redis", the only thing to change is to add one line in the conf file "syslog-ident".

[zuiderkwast]

@hwware Of course, you're right! A user can just set syslog-ident to "redis".

By brain was overcomplicating it.

[karthyuom]

An idea: Maybe we can set the default value at createStringConfig("syslog-ident", ...) to NULL.

Thanks for the suggestion @zuiderkwast. if we need to set it to NULL, then I realized that we also need to change the ALLOW_EMPTY_STRING to EMPTY_STRING_IS_NULL. But, then it will change the existing behavior where user provided empty-string for the syslog-ident can also be changed to "redis" or "valkey" depending on the extended compatibility config.

Therefore, I have pushed another solution which can work with the extended compatibility config. Also, added unit tests to cover this change.

[zuiderkwast]

@karthyuom Thanks for solving this fancy logic, but I actually think the simple solution suggested by @hwware is better. What matters is that it's possible to configure it to be redis compatible.

We don't want to add unnecessary complexity for this temporary compatibility-config that we will remove in a few versions anyway.

[karthyuom]

@zuiderkwast Yes I agree. Then I can revert back to the original commit where user can even configure "redis" for backward compatibility. Thanks @hwware for bringing this up.

[zuiderkwast]

Yes just use SERVER_NAME for the default value.

[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