feat: added new cache package for query service

[vikrantgupta25]

Summary

• implement new cache package for query service caching
• the inmemory cache now stores the value as is rather than ser/deser the values
• the cacheable entity provides interfaces to add custom MarshalBinary and UnmarshalBinary to handle binary encodings for redis
• updated makefile for running all pkg tests
• added new signoz package for setting up the dependencies

Related Issues / PR's

contributes to - #6132

Screenshots

NA

Affected Areas and Manually Tested Areas

---

Important

Introduces a new caching package for the query service with in-memory and Redis support, updates configuration, and adds tests.

• Caching:
  • Introduces pkg/cache package for query service caching.
  • Supports in-memory and Redis caching strategies in pkg/cache/strategy/memory and pkg/cache/strategy/redis.
  • CacheableEntity interface for custom binary encoding.
  • Updates server.go to integrate caching with ServerOptions.
• Configuration:
  • Adds cache configuration to conf/defaults.yaml and pkg/config/config.go.
  • Updates pkg/web/config.go to include skip_web_frontend option.
• Testing:
  • Adds tests for memory and Redis caching strategies in memory_test.go and redis_test.go.
• Misc:
  • Updates Makefile to run all package tests.
  • Adds pkg/signoz package for dependency setup.

^{This description was created by for 5d15019. It will automatically update as commits are pushed.}

[github-actions]

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

[github-actions]

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

[github-actions]

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to eb7c30d in 1 minute and 41 seconds

More details

• Looked at 817 lines of code in 9 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. pkg/cache/strategy/redis/redis.go:44

• Draft comment:
  Consider adding a check to ensure data is a non-nil pointer before proceeding. This will prevent potential runtime errors when data is nil or not a pointer.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:

1. The data parameter is an interface type, so runtime type checking would be handled by Go's type system. 2. If nil values are not allowed, this should be documented in the interface definition. 3. The redis client's Set method likely has its own validation. 4. Adding nil checks everywhere can lead to defensive programming that doesn't add real value.
   I might be underestimating the value of defensive programming. A nil check could prevent issues in development before they reach production.
   While defensive programming has its place, in this case it would be better to document any constraints in the interface definition rather than add runtime checks everywhere the interface is used.
   The comment should be removed as it suggests adding unnecessary defensive programming that would be better handled through interface documentation and design.

2. pkg/cache/strategy/redis/redis.go:45

• Draft comment:
  Consider adding a check to ensure data is a non-nil pointer before proceeding. This will prevent potential runtime errors when data is nil or not a pointer.
• Reason this comment was not posted:
  Marked as duplicate.

3. pkg/cache/strategy/redis/redis.go:49

• Draft comment:
  Consider adding a check to ensure dest is a non-nil pointer before proceeding. This will prevent potential runtime errors when dest is nil or not a pointer.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:

1. The redis.Scan method likely already handles nil cases. 2. If dest is an interface, proper usage should be documented in the interface definition. 3. Adding nil checks everywhere can add noise to the code. 4. This is a new file, but the comment isn't about a specific change - it's a general suggestion.
   The comment raises a valid concern about runtime safety. A nil check could prevent cryptic errors from bubbling up from the redis package.
   While runtime safety is important, this check belongs in the interface documentation or as validation in the entity.CacheableEntity interface implementation, not here.
   Delete the comment as it suggests defensive programming that would be better handled at the interface level, and it's not specifically related to a change in the code.

4. pkg/cache/strategy/redis/redis.go:50

• Draft comment:
  Consider adding a check to ensure dest is a non-nil pointer before proceeding. This will prevent potential runtime errors when dest is nil or not a pointer.
• Reason this comment was not posted:
  Marked as duplicate.

5. pkg/cache/strategy/memory/memory.go:38

• Draft comment:
  Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This comment applies to other similar instances in the code as well.
• Reason this comment was not posted:
  Comment was not on a valid diff hunk.

Workflow ID: wflow_EgFD95nBKX6JFBmI

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on d1ddbc9 in 1 minute and 11 seconds

More details

• Looked at 857 lines of code in 14 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. pkg/cache/strategy/memory/memory.go:94

• Draft comment:
  The Configuration method should return the actual cache configuration instead of nil.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. pkg/cache/strategy/redis/redis.go:93

• Draft comment:
  Implement a Configuration method to return the Redis cache configuration, similar to the memory strategy.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. pkg/cache/strategy/redis/redis.go:19

• Draft comment:
  Add a check for nil opts in the New function to prevent potential panics.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment has merit since a nil opts would cause a panic in Connect() when accessing opts.Host etc. However, this is a constructor and the type *_cache.Redis likely comes from a higher level package that may already handle initialization. Without seeing the _cache package, I can't be 100% certain if this check is necessary or redundant.
  I don't have visibility into the _cache package to know if opts is guaranteed to be non-nil when passed in. The change from internal Options to _cache.Redis suggests this may be handled at a higher level.
  While the nil check would prevent panics, suggesting it without understanding the broader context could lead to redundant checks. The migration to _cache.Redis type hints at better structure.
  Given the lack of full context and following the principle of removing uncertain comments, this comment should be deleted.

4. pkg/signoz/signoz.go:1

• Draft comment:
  Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_UgTgwO6EPk9DlWSZ

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on cd70178 in 21 seconds

More details

• Looked at 22 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/signoz/signoz.go:13

• Draft comment:
  The change from a pointer to a value for the Cache field in the SigNoz struct is correct, given the context of the changes. However, ensure that the cache package handles the instantiation of specific strategies internally, based on configuration, to avoid unnecessary imports.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The change from a pointer to a value for the Cache field in the SigNoz struct is correct, given the context of the changes. However, the import statements for specific cache strategies (memory and redis) are unnecessary in this file since they are not directly used. The cache package should handle the instantiation of specific strategies internally, based on configuration.

2. pkg/signoz/signoz.go:10

• Draft comment:
  Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
• Reason this comment was not posted:
  Comment was not on a valid diff hunk.

Workflow ID: wflow_F7cyIKpwWbkuTbfE

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 622aebd in 34 seconds

More details

• Looked at 876 lines of code in 14 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/query-service/utils/format.go:233

• Draft comment:
  The function AddBackTickToFormatTag is defined twice in this file. Please remove the redundant definition.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. pkg/query-service/utils/format.go:233

• Draft comment:
  The functions AddBackTickToFormatTag and AddBackTickToFormatTags are correctly implemented for formatting strings with backticks. No changes needed here.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The function AddBackTickToFormatTag and AddBackTickToFormatTags are used to format strings by adding backticks if they contain a dot. This is a utility function and is correctly placed in the utils package. However, there are no issues with the implementation of these functions.

Workflow ID: wflow_TCehXEcReXO16xl3

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on dd24696 in 24 seconds

More details

• Looked at 21 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. pkg/cache/config.go:47

• Draft comment:
  The Validate function is currently a no-op. Consider implementing validation logic to ensure that the configuration is correct and complete, such as checking if the Provider is either "memory" or "redis" and validating Redis connection details.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_vyfKU81H3uK6kraI

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 649a4ca in 14 seconds

More details

• Looked at 10 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. Makefile:193

• Draft comment:
  The test target now runs tests for all subdirectories under pkg, not just query-service. Ensure this broader scope is intentional and that all tests are expected to pass.
• Reason this comment was not posted:
  Comment did not seem useful.

2. Makefile:193

• Draft comment:
  The change to go test ./pkg/... is appropriate for testing all packages under ./pkg/. No issues here.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The Makefile change is straightforward, expanding the test command to cover all packages under ./pkg/. This is a valid change and doesn't violate any of the specified rules.

Workflow ID: wflow_rzH8HtsqbcJMYf1A

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 04350bc in 39 seconds

More details

• Looked at 111 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/config/config_test.go:40

• Draft comment:
  The Memory cache configuration is set even though the provider is redis. This is inconsistent. Consider removing the Memory cache configuration when the provider is set to redis.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  This is a test file, and the expected config might be reflecting the actual behavior of the config parsing system. Without seeing the actual config parsing code, we can't be sure if having both configurations is actually wrong - it could be that the system maintains default memory settings even when using Redis. The comment is making assumptions about the intended design without strong evidence.
  I might be too lenient - having unused configuration could lead to confusion. Also, this could be a genuine bug in the test expectations.
  However, this is a test file that likely reflects the actual behavior of the system. Without seeing the implementation, we can't be certain this is incorrect - it could be intentional to maintain default memory settings.
  Delete the comment because we don't have strong evidence that having both configurations is actually wrong - it could be working as designed.

2. pkg/config/config_test.go:16

• Draft comment:
  Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This comment applies to the entire file.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The test file is correctly structured and does not violate any of the specified rules.

Workflow ID: wflow_Tja1Pbg1dcAWWSxj

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4b8aa0c in 38 seconds

More details

• Looked at 468 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. pkg/cache/strategy/memory/memory.go:22

• Draft comment:
  Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This comment applies to similar patterns in other files as well.
• Reason this comment was not posted:
  Comment was not on a valid diff hunk.

Workflow ID: wflow_OxPJ2Bnl7sujVQSm

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[grandwizard28]

@vikrantgupta25 we overrode the skipWebFrontend change here.

We can pass this flag in signoz.New(config, skipWebFrontend) and have an action item on me to incorporate this better for now!

[vikrantgupta25]

Added it to the web config itself to keep things centralised in place and to avoid messing up the design pattern

[ellipsis-dev]

👍 Looks good to me! Incremental review on 53f4bda in 52 seconds

More details

• Looked at 104 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. pkg/web/config.go:18

• Draft comment:
  Consider using a boolean type for SkipWebFrontend instead of a string. This will prevent potential errors and improve code clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment points out something that the author is already aware of and has explicitly documented. The limitation is with the confmap package not handling boolean flags properly yet. Making this change isn't currently possible, so the comment isn't actionable.
  Maybe there's a workaround for the boolean flag issue that the author hasn't considered? The TODO might be outdated.
  The TODO appears to be part of the same change, so it's current. Without deeper knowledge of the confmap package's limitations, suggesting workarounds would be speculative.
  Delete the comment because it suggests something the author is already aware of but can't implement due to technical limitations explicitly documented in a TODO.

2. pkg/web/web.go:31

• Draft comment:
  Consider using a boolean type for SkipWebFrontend instead of a string. This will prevent potential errors and improve code clarity.
• Reason this comment was not posted:
  Marked as duplicate.

3. ee/query-service/app/server.go:388

• Draft comment:
  Consider using a boolean type for SkipWebFrontend instead of a string. This will prevent potential errors and improve code clarity.
• Reason this comment was not posted:
  Marked as duplicate.

4. pkg/web/config.go:19

• Draft comment:
  Consider using a boolean for SkipWebFrontend once confmap supports it. Currently, it's a string due to limitations.
• Reason this comment was not posted:
  Confidence changes required: 33%
  The code uses a string to represent a boolean value for SkipWebFrontend. This is not ideal, but the comment indicates it's a temporary workaround due to a limitation in confmap. No action is needed here.

Workflow ID: wflow_W3tEb8p1U8VzWxoC

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 5d15019 in 36 seconds

More details

• Looked at 16 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. pkg/config/config_test.go:37

• Draft comment:
  SkipWebFrontend should be a boolean instead of a string to avoid potential issues if the implementation expects a boolean value.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  Since this is a test file verifying environment variable parsing, the string type likely matches the actual implementation since env vars are always strings. The test is just verifying the raw parsed value. Changing to boolean would actually make the test incorrect since it wouldn't match the real behavior. The comment shows a misunderstanding of how environment variable configuration works.
  Maybe there's a config parsing layer that converts string env vars to typed values that I'm not seeing? The actual web.Config struct definition isn't visible here.
  Even if there is type conversion happening elsewhere, this test is specifically about the raw environment variable parsing stage, where values are always strings. The comment is still incorrect in this context.
  Delete this comment. It shows a misunderstanding of environment variable parsing. String values are correct here since this test verifies the raw env var parsing.

Workflow ID: wflow_B4gF0kq10ygxXIEs

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.