Skip to content

Use Hive Logger v1 alpha release - #56

Draft
enisdenjo wants to merge 2 commits into
dahlia:mainfrom
enisdenjo:hiveloggeralpha
Draft

Use Hive Logger v1 alpha release#56
enisdenjo wants to merge 2 commits into
dahlia:mainfrom
enisdenjo:hiveloggeralpha

Conversation

@enisdenjo

Copy link
Copy Markdown

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?

@newfadel newfadel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/logger dependency across package.json, deno.json, and pnpm-lock.yaml to use a specific alpha release (1.0.0-alpha-b59321b772f970ee1e46d2ffc249e59427634cb7) instead of the previously referenced ^1.0.0 stable version, correcting an accidental release.
  • Benchmark Documentation Alignment: Modified benchmarks/README.md and docs/comparison.md to 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.ts to remove custom AttributeValue and Attributes type definitions, now directly utilizing the hiveLogger.Attributes type, 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

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Update Benchmarks (Preferred): Re-run all benchmarks on a consistent environment using the new alpha version and update the tables in both benchmarks/README.md and docs/comparison.md with the new results.
  2. 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 in docs/comparison.md (around line 220). This note should clearly state that the displayed results for "Hive Logger (alpha)" are currently from the previous 1.0.0 version and do not yet reflect the performance of the 1.0.0-alpha version. An example disclaimer is provided in the specific file comments.

Addressing this will ensure the documentation remains accurate and trustworthy.

Comment thread benchmarks/README.md
| 7 | Signale | 2,700.00 | 17.13x slower |
| Rank | Library | Time (ns/iter) | Relative Speed |
| ---- | ------------------- | -------------: | --------------- |
| 1 | Hive Logger (alpha) | 157.64 | 1.00x (fastest) |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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:

  1. Update Benchmark Figures: Re-run the benchmarks with the new alpha version and update all corresponding figures in this README.
  2. 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.0 version 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.

Comment thread docs/comparison.md
| winston | 701 | 757 | 569 |
| Library | Node.js | Deno | Bun |
| ------------------- | ------: | ------: | ------: |
| Hive Logger (alpha) | 158 | 2,390 | 157 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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:

  1. Update Benchmark Figures: Re-run the null benchmark with the alpha version and update these numbers accordingly.
  2. 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.0 version 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants