Use Hive Logger v1 alpha release - #56
Conversation
newfadel
left a comment
There was a problem hiding this comment.
The PR description mentions that the stable 1.0.0 release of @graphql-hive/logger was an accident and does not use the latest code. This implies that the intention is to use the v1-alpha release because it contains the most up-to-date code.
Given this context, a professional code review comment for the PR could focus on verifying that the v1-alpha release is indeed being used and that the benchmarks are updated to reflect the changes. It's also important to address the point about running on different machines.
Here's a possible comment:
"Thanks for this PR! It's good to see the update to Hive Logger v1-alpha, especially given the note about the 1.0.0 release.
Could you please confirm that the benchmarks/deno.json and benchmarks/package.json files are correctly configured to use the v1-alpha release of @graphql-hive/logger?
Regarding the benchmark results, since they were run on different machines, it would be best to re-run them on a consistent environment (e.g., a CI pipeline or a specific machine with documented specs) to ensure consistent and comparable data for benchmarks/README.md and docs/comparison.md. This will give us a more accurate understanding of the performance impact.
Let me know if you need any assistance with setting up a consistent environment for re-running the benchmarks.
There was a problem hiding this comment.
Summary of Changes
Hello @enisdenjo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on correcting the version of the Hive Logger library used in the project's benchmarks and documentation. It updates the dependency to a specific alpha release, which contains the intended latest code, and adjusts related files to reflect this change, including minor code adaptations for the new library API.
Highlights
- Dependency Update: Updated the
@graphql-hive/loggerdependency acrosspackage.json,deno.json, andpnpm-lock.yamlto use a specific alpha release (1.0.0-alpha-b59321b772f970ee1e46d2ffc249e59427634cb7) instead of the previously referenced^1.0.0stable version, correcting an accidental release. - Benchmark Documentation Alignment: Modified
benchmarks/README.mdanddocs/comparison.mdto explicitly label 'Hive Logger' as 'Hive Logger (alpha)' in all benchmark results and version listings, ensuring documentation accurately reflects the version being tested. - Code Refinement: Refactored
benchmarks/null.tsto remove customAttributeValueandAttributestype definitions, now directly utilizing thehiveLogger.Attributestype, indicating an API change in the updated alpha version of the logger.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR correctly updates @graphql-hive/logger to the specified alpha version (1.0.0-alpha-b59321b772f970ee1e46d2ffc249e59427634cb7) across the benchmark configurations in package.json, deno.json, and the pnpm-lock.yaml file. The type simplification in benchmarks/null.ts, by using hiveLogger.Attributes from the updated library, is also a good improvement.
Regarding your question about the benchmark results in benchmarks/README.md and docs/comparison.md:
The current results are from the previous 1.0.0 version. Since the library code has changed with the alpha release, these results are likely no longer representative. To maintain accuracy and avoid misleading readers, I strongly recommend one of the following actions:
- Update Benchmarks (Preferred): Re-run all benchmarks on a consistent environment using the new alpha version and update the tables in both
benchmarks/README.mdanddocs/comparison.mdwith the new results. - Add Clear Disclaimers: If re-running benchmarks immediately isn't feasible, add a prominent note at the beginning of the 'Benchmark results' section in
benchmarks/README.md(around line 32) and near the table indocs/comparison.md(around line 220). This note should clearly state that the displayed results for "Hive Logger (alpha)" are currently from the previous1.0.0version and do not yet reflect the performance of the1.0.0-alphaversion. An example disclaimer is provided in the specific file comments.
Addressing this will ensure the documentation remains accurate and trustworthy.
| | 7 | Signale | 2,700.00 | 17.13x slower | | ||
| | Rank | Library | Time (ns/iter) | Relative Speed | | ||
| | ---- | ------------------- | -------------: | --------------- | | ||
| | 1 | Hive Logger (alpha) | 157.64 | 1.00x (fastest) | |
There was a problem hiding this comment.
The label has been updated to 'Hive Logger (alpha)', but the benchmark data (e.g., 157.64 ns/iter) appears to be from the previous 1.0.0 version, as mentioned in the PR description. This could be misleading as the underlying code for the alpha version has changed.
To address this, please consider one of the following:
- Update Benchmark Figures: Re-run the benchmarks with the new alpha version and update all corresponding figures in this README.
- Add a Prominent Disclaimer: If updating the figures immediately is not possible, add a clear note at the beginning of the 'Benchmark results' section (e.g., around line 32). This note should explicitly state that the current numbers for 'Hive Logger (alpha)' are based on the old
1.0.0version and will be updated. For example:**Note:** The benchmark results below for "Hive Logger (alpha)" currently display data from the previous `1.0.0` version. These results have not yet been updated to reflect the performance of the `1.0.0-alpha` version and will be updated in a future commit.
| | winston | 701 | 757 | 569 | | ||
| | Library | Node.js | Deno | Bun | | ||
| | ------------------- | ------: | ------: | ------: | | ||
| | Hive Logger (alpha) | 158 | 2,390 | 157 | |
There was a problem hiding this comment.
The table now lists 'Hive Logger (alpha)', but the associated performance data (Node.js: 158, Deno: 2,390, Bun: 157) is from the older 1.0.0 version, as indicated in the PR description. This could mislead readers about the alpha version's actual performance characteristics.
To ensure clarity, please either:
- Update Benchmark Figures: Re-run the null benchmark with the alpha version and update these numbers accordingly.
- Add a Disclaimer: Place a note directly above this table (e.g., around line 220) clarifying that the Hive Logger results are for the
1.0.0version and not the new alpha. For instance:**Note:** The benchmark results below for "Hive Logger (alpha)" currently display data from the previous `1.0.0` version. These results have not yet been updated to reflect the performance of the `1.0.0-alpha` version and will be updated in a future commit.
The stable 1.0.0 release was an accident and does not use the latest code.
Also I didn't update the results since we're running on different machines. How would you like me to proceed?