Skip to content
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.

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

[Serve] Add actor id and worker id to Serve structured logs #43725

Merged

Conversation

GeneDer
Copy link
Contributor

@GeneDer GeneDer commented Mar 5, 2024

Why are these changes needed?

Add actor_id and worker_id to Serve structured json logs.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Signed-off-by: Gene Su <e870252314@gmail.com>
@GeneDer
Copy link
Contributor Author

GeneDer commented Mar 5, 2024

Also tested manually seeing those new fields are now logged
image

@GeneDer GeneDer self-assigned this Mar 5, 2024
Comment on lines +97 to +99
for field in ServeJSONFormatter.ADD_IF_EXIST_FIELDS:
if field in record_attributes:
record_format[field] = record_attributes[field]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the attributes added here can be included in the base record_format that's generated in the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require us to pass more things to the constructor, but I guess it makes more sense there since they won't change between each log messages. Let me update it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

These ones are just calling get_runtime_context() anyways

Copy link
Contributor

Choose a reason for hiding this comment

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

can't you just call it in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's right. Let me just call those in init 😅

Signed-off-by: Gene Su <e870252314@gmail.com>
Copy link
Contributor

@edoakes edoakes left a comment

Choose a reason for hiding this comment

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

Please add a defensive check that the get_runtime_context() call works and contains the relevant fields (we can omit them if it fails, i.e., if we are outside an actor)

Signed-off-by: Gene Su <e870252314@gmail.com>
…or_id() and get_worker_id() in case if they fails

Signed-off-by: Gene Su <e870252314@gmail.com>
Comment on lines 68 to 82
SERVE_LOG_ACTOR_ID: runtime_context.get_actor_id(),
SERVE_LOG_WORKER_ID: runtime_context.get_worker_id(),
}
try:
runtime_context = ray.get_runtime_context()
actor_id = runtime_context.get_actor_id()
if actor_id:
self.component_log_fmt[SERVE_LOG_ACTOR_ID] = actor_id
worker_id = runtime_context.get_worker_id()
if worker_id:
self.component_log_fmt[SERVE_LOG_WORKER_ID] = worker_id
except Exception:
# If get_runtime_context() fails for any reason, do nothing (no adding
# actor_id and/or worker_id to the fmt)
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you probably meant to put the try-except block above the dict construction :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I meant to remove them in line 68-69 and just have they added to the dictionary in the try-except block 😅

Signed-off-by: Gene Su <e870252314@gmail.com>
f'"request_id": "{resp["request_id"]}", '
f'"route": "{resp["route"]}", '
f'"application": "{resp["app_name"]}", "message":.* user func.*'
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the message fields removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just reordered to before the request id. This is due to my refactor for drying up the ADD_IF_EXIST_FIELDS thing. If we do want message to go last I can do move it down. Just feel since it's structured json, the order shouldn't matter

Copy link
Contributor

Choose a reason for hiding this comment

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

nope doesn't matter

Signed-off-by: Gene Su <e870252314@gmail.com>
@edoakes edoakes merged commit 245df53 into ray-project:master Mar 6, 2024
9 checks passed
@GeneDer GeneDer deleted the add-more-serve-structured-log-fields branch March 6, 2024 17:24
ryanaoleary pushed a commit to ryanaoleary/ray that referenced this pull request Jun 7, 2024
…ect#43725)

Add actor_id and worker_id to Serve structured json logs.

---------

Signed-off-by: Gene Su <e870252314@gmail.com>
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.

None yet

2 participants