[otelfiber] Allow for disabling client IP collection in traces #1033
Conversation
gzuidhof
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Rate Limit Exceeded@gzuidhof has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 40 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent update introduces a new feature to control the collection of client IP addresses in the otelfiber package. A boolean field, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- otelfiber/config.go (2 hunks)
- otelfiber/fiber.go (1 hunks)
- otelfiber/otelfiber_test/fiber_test.go (1 hunks)
- otelfiber/semconv.go (1 hunks)
Files skipped from review due to trivial changes (1)
- otelfiber/fiber.go
Additional comments: 4
otelfiber/semconv.go (1)
- 59-63: The implementation of the conditional logic to collect the client's IP address based on the
cfg.collectClientIPsetting is correct and aligns with the PR's objectives. This change effectively allows users to disable the collection of client IP addresses, enhancing privacy controls.otelfiber/config.go (2)
- 21-21: The addition of the
collectClientIPboolean field to theconfigstruct is correctly implemented. This field enables the new feature to allow or prevent the collection of client IP addresses, aligning with the PR's objectives.- 101-106: The
WithCollectClientIPfunction is correctly implemented, allowing users to configure the collection of client IP addresses. This addition follows the established pattern for configuration options in the package and is well-documented, clearly explaining its purpose and default behavior.otelfiber/otelfiber_test/fiber_test.go (1)
- 500-536: The
TestCollectClientIPfunction is well-implemented, providing comprehensive test coverage for the new feature under both enabled and disabled conditions. The use of parallel test execution and dynamic test names enhances the test suite's efficiency and readability.
| if len(clientIP) > 0 { | ||
| attrs = append(attrs, semconv.HTTPClientIPKey.String(utils.CopyString(clientIP))) | ||
| if cfg.collectClientIP { | ||
| clientIP := c.IP() |
There was a problem hiding this comment.
This is unchanged from the original, if that needs to be changed probably that makes more sense in a different PR.
|
I wasn't sure how to update the README.. The README currently describes the |
Fair! |
|
@gzuidhof Why is the config setting private and can only be changed using the help function? Regardless of the answer, please also include the help function with signature and explanation in the readme Every feature without examples or explanation gives the user fewer hints and is therefore questioned, is not known or is not used |
|
I'm happy for you (or anybody else) to make changes to this PR as you see fit :) There is no mechanism for passing in your own The discoverability for these functional parameters are in your language server suggestions.. Anyhow: happy for you to make any changes, just explaning my reasons :) |
|
okay thanks for the explanation can you include the signatures of the |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- otelfiber/README.md (1 hunks)
Additional comments: 1
otelfiber/README.md (1)
- 34-47: The documentation for the configuration options has been updated to reflect the new functional parameter style and the addition of the
WithCollectClientIPoption. While the changes are generally clear and informative, there are a few areas that could be improved for better readability and consistency:
- Ensure consistent capitalization in descriptions. For example, "specifies" should be capitalized when it starts a sentence.
- Add a period at the end of each description for consistency.
- The description for
WithPortmentions "Required: If not default..." which could be clearer. Consider rephrasing to indicate that specifying the port is necessary unless the default port is used.Additionally, consider adding a brief example or code snippet demonstrating how to disable the client IP collection using the
WithCollectClientIPoption. This would help users quickly understand how to apply the new feature.
|
Done! :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- otelfiber/README.md (1 hunks)
Additional comments: 1
otelfiber/README.md (1)
- 34-47: The documentation has been updated to reflect the new configuration options introduced in the PR. However, there are a few areas that could be improved for clarity and consistency:
The description for
WithPortmentions "Required: If not default (80forhttp,443forhttps)" which could be misleading since it suggests that the port is a required parameter, whereas it actually has a default behavior. Consider rephrasing to clarify that the default port is used unless specified otherwise.The introduction of
WithCollectClientIPis a significant change. It would be beneficial to include an example or a more detailed explanation of how to use this option, especially since it pertains to privacy concerns.There are minor typographical issues throughout the table, such as inconsistent use of spaces and capitalization. Ensuring consistency in formatting will improve the readability of the documentation.
Given the feedback from the PR comments about enhancing user understanding through examples, it might be helpful to expand the "Example" section to demonstrate the use of the new
WithCollectClientIPoption, along with other configuration options.Consider the following improvements to enhance the documentation:
- Clarify the description of
WithPortto avoid confusion about it being a required parameter.- Provide a detailed example or explanation for
WithCollectClientIP, highlighting its importance for privacy.- Address typographical and formatting inconsistencies for a cleaner presentation.
- Expand the "Example" section to include usage of the new configuration option and others, demonstrating their practical application.
|
thx for the adjustment from our ai |
|
I addressed them in this commit, I think it hasn't refreshed |
|
ok, thx |
|
@coderabbitai can you pls check the last state again |
|
Tip For best results, initiate chat on the files or code changes. @ReneWerner87, I've reviewed the latest state of the PR, specifically focusing on the README.md file and the latest commit However, based on the provided output, it appears that the specific details regarding the To ensure comprehensive documentation and transparency for users, it might be beneficial to verify that the
This approach will help in enhancing discoverability and usability of the new features introduced in the PR, aligning with the objectives of providing users with more control and flexibility. If further detailed review or actions are needed, please let me know! 🐇 Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Fetch the latest commit hash from the PR branch
git log -n 1 --pretty=format:"%H"
# Fetch the README file content to review the changes made
cat README.md
Length of output: 5304 |
|
ok, we improve the readme later with examples -> follow up PR ? |
Introduces a
WithCollectClientIP(collect bool)that disables IP collection into traces.Summary by CodeRabbit