Skip to content
Snippets Groups Projects

Add more extensive logging guarded by feature flag

Merged Gosia Ksionek requested to merge mk-add-logs into master
1 unresolved thread

What does this MR do and why?

Add logging every log of LLM cycle. It is guarded by the feature flag scoped to user.

Some explanation: I was thinking of overriding debug method in Llm::Logger class and adding the condition there. I decided against it as it would made the logic a bit hidden and making it less readable.

In the course of the review, I created info_or_debug method that encapsulates the feature flag logic.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. In rails console enable the experiment fully
    Feature.enable(:expanded_ai_loggging)
  2. Ask some question in chat.
  3. check if entries in llm.log are logged on info level.
  4. Switch feature flag off, ask question again and compare llm.log file content.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

solves https://gitlab.com/gitlab-org/gitlab/-/issues/432688

Edited by Gosia Ksionek

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Contributor
    2 Warnings
    :warning: 44020675: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
    :warning: 85740886: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
    1 Message
    :book: CHANGELOG missing:

    If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    backend @thutterer profile link current availability (UTC+1, same timezone as author) @jarka profile link current availability (UTC+1, same timezone as author)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Gosia Ksionek resolved all threads

    resolved all threads

  • Gosia Ksionek added 1 commit

    added 1 commit

    • 43696dfc - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for bea7983e

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Monitor          | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Create           | 50     | 0      | 7       | 0     | 57    | ✅     |
    | Plan             | 55     | 0      | 0       | 0     | 55    | ✅     |
    | Verify           | 31     | 0      | 0       | 0     | 31    | ✅     |
    | Govern           | 57     | 0      | 0       | 0     | 57    | ✅     |
    | Data Stores      | 23     | 0      | 0       | 0     | 23    | ✅     |
    | Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Manage           | 0      | 0      | 1       | 0     | 1     | ➖     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 224    | 0      | 10      | 0     | 234   | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :heavy_minus_sign: test report for bea7983e

    expand test summary
    +------------------------------------------------------------+
    |                       suites summary                       |
    +-------+--------+--------+---------+-------+-------+--------+
    |       | passed | failed | skipped | flaky | total | result |
    +-------+--------+--------+---------+-------+-------+--------+
    +-------+--------+--------+---------+-------+-------+--------+
    | Total | 0      | 0      | 0       | 0     | 0     | ➖     |
    +-------+--------+--------+---------+-------+-------+--------+
  • Author Maintainer

    @nicolasdular would you review please?

  • Gosia Ksionek requested review from @nicolasdular

    requested review from @nicolasdular

  • Nicolas Dular approved this merge request

    approved this merge request

  • Author Maintainer

    Pipeline failure due to broken master

  • Author Maintainer

    @jprovaznik would you mind performing maintainer review?

  • Gosia Ksionek requested review from @jprovaznik

    requested review from @jprovaznik

    • Resolved by Jan Provaznik

      I was thinking of overriding debug method in Llm::Logger class and adding the condition there. I decided against it as it would made the logic a bit hidden and making it less readable.

      @mksionek I was thinking that we could rather define temporarily Llm::Logger.info_or_debug method, so it would be still quite clear and we would avoid all if-clauses.

      But because this MR is already prepared and it's expected to be present for a short period of time, I think it's acceptable to keep it as is.

  • Jan Provaznik resolved all threads

    resolved all threads

  • Jan Provaznik approved this merge request

    approved this merge request

  • Jan Provaznik removed review request for @jprovaznik

    removed review request for @jprovaznik

  • Gosia Ksionek added 2 commits

    added 2 commits

    Compare with previous version

  • Gosia Ksionek added 1 commit

    added 1 commit

    Compare with previous version

  • Gosia Ksionek requested review from @jprovaznik

    requested review from @jprovaznik

  • Gosia Ksionek changed the description

    changed the description

  • Jan Provaznik approved this merge request

    approved this merge request

  • Jan Provaznik resolved all threads

    resolved all threads

  • Thanks @mksionek, LGTM :thumbsup:, MWPS set :rocket:

  • Jan Provaznik enabled an automatic merge when the pipeline for cf2cb754 succeeds

    enabled an automatic merge when the pipeline for cf2cb754 succeeds

  • merged

  • Jan Provaznik mentioned in commit ff95c9c2

    mentioned in commit ff95c9c2

  • added workflowstaging label and removed workflowcanary label

  • mentioned in issue mksionek/mksionek#33

  • mentioned in issue #433001 (closed)

  • Gosia Ksionek mentioned in merge request !155146 (closed)

    mentioned in merge request !155146 (closed)

  • Please register or sign in to reply
    Loading