Skip to content

Add pilot logging #550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add pilot logging #550

wants to merge 14 commits into from

Conversation

Robin-Van-de-Merghel
Copy link
Contributor

Updates to #511 , rebased and linked to #421

@Robin-Van-de-Merghel Robin-Van-de-Merghel marked this pull request as draft May 22, 2025 07:28
@Robin-Van-de-Merghel Robin-Van-de-Merghel force-pushed the robin-pilot-logging branch 2 times, most recently from 5218531 to 9d7e6b1 Compare May 23, 2025 15:00
This was referenced May 23, 2025
Closed
Open
@Robin-Van-de-Merghel Robin-Van-de-Merghel force-pushed the robin-pilot-logging branch 4 times, most recently from 87ffdf6 to 50e9ff3 Compare May 27, 2025 09:09
@Robin-Van-de-Merghel Robin-Van-de-Merghel marked this pull request as ready for review May 27, 2025 09:09
@Robin-Van-de-Merghel
Copy link
Contributor Author

To review this code, this is a summary of what's new compared to the pilot authentification:

  • diracx-routers/src/diracx/routers/pilots/logging.py: Upload messages
  • diracx-routers/src/diracx/routers/pilot_management/query.py: There's query_logs which helps querying logs with the same idea as job queries
  • diracx-logic/src/diracx/logic/pilots/logging.py: Tools to update and query logs
  • Modified diracx-testing/src/diracx/testing/mock_osdb.py and util.py to test OSDB with new types: date_nanos. Also added support for queries: returned total (for Content-Range).
  • diracx-core/src/diracx/core/models.py: Added logs
  • diracx-db/src/diracx/db/*.py: Added logs and updated debug (when we add logs)

@Robin-Van-de-Merghel Robin-Van-de-Merghel force-pushed the robin-pilot-logging branch 3 times, most recently from 7dc9b17 to 86e715d Compare June 18, 2025 10:48
@martynia
Copy link
Contributor

I'm wondering why you removed the original line number from the send_message when storing a document. You added a timestamp which is useful, but it might be problematic when sorting messages. The messages should be presented in the order they were emitted by a client.

@Robin-Van-de-Merghel
Copy link
Contributor Author

Robin-Van-de-Merghel commented Jun 19, 2025 •
edited
Loading

I'm wondering why you removed the original line number from the send_message when storing a document. You added a timestamp which is useful, but it might be problematic when sorting messages. The messages should be presented in the order they were emitted by a client.

As timestamps are generated from the pilot itself (as logs are printed out), if we sort logs by timestamps, we could add the right numbering without losing space?

Sorting by timestamp would actually give logs in order they were caught by a pilot, which is what we want? If for whatever reason numbering is broken, we could have shuffled logs, whereas with timestamps we are assured order is kept.

@fstagni fstagni mentioned this pull request Jun 20, 2025
Open
@martynia
Copy link
Contributor

To get the numberring it would be sufficient to define a class variable in the extended logger in the pilot and increment it every time a log statement is called.

Robin-Van-de-Merghel
Robin-Van-de-Merghel commented Jun 27, 2025
View reviewed changes
diracx-core/src/diracx/core/models.py Outdated


class LogMessage(BaseModel):
pilot_stamp: str
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we still need that ? Because now the token already includes the stamp

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, as long as you can get back a correct pilot from PilotAgentsDB, then the stamp is superfluous.

diracx-db/src/diracx/db/os/pilot_logs.py Outdated
index_prefix = "pilot_logs"

def index_name(self, vo: str, doc_id: int) -> str:
# TODO decide how to define the index name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modify to use same splitting idea as jobs: pilot_logs_..._m

@Robin-Van-de-Merghel Robin-Van-de-Merghel force-pushed the robin-pilot-logging branch 4 times, most recently from 2f03c17 to e409e43 Compare July 3, 2025 07:46
martynia
martynia reviewed Jul 14, 2025
View reviewed changes
diracx-routers/tests/pilots/test_pilot_logging.py
lines.append(
{
"message": line,
"timestamp": "2022-02-26 13:48:35.123456",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering how you can get those 2 lines back in the right order if the timestamp is identical.?

@Robin-Van-de-Merghel Robin-Van-de-Merghel changed the title Robin pilot logging Add pilot logging Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@martynia martynia martynia left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Robin-Van-de-Merghel @martynia @chrisburr