-
Notifications
You must be signed in to change notification settings - Fork 18
Remote and requests 20221026 #190
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| """ | ||
| This file contains Django middleware that can be inserted into an app's stack to expose the remote | ||
| recording endpoint. | ||
|
|
||
| It expects the importer to have verified that Django is available. | ||
| """ | ||
|
|
||
| from http import HTTPStatus | ||
|
|
||
| from django.http import HttpRequest, HttpResponse | ||
|
|
||
| from . import remote_recording | ||
|
|
||
|
|
||
| class RemoteRecording: # pylint: disable=missing-class-docstring | ||
| def __init__(self, get_response): | ||
| super().__init__() | ||
| self.get_response = get_response | ||
|
|
||
| def __call__(self, request: HttpRequest): | ||
| if request.path != "/_appmap/record": | ||
| return self.get_response(request) | ||
|
|
||
| handlers = { | ||
| "GET": remote_recording.status, | ||
| "POST": remote_recording.start, | ||
| "DELETE": remote_recording.stop, | ||
| } | ||
|
|
||
| def not_allowed(): | ||
| return "", HTTPStatus.METHOD_NOT_ALLOWED | ||
|
|
||
| body, status = handlers.get(request.method, not_allowed)() | ||
| return HttpResponse(body, status=status, content_type="application/json") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,39 +1,30 @@ | ||
| """ remote_recording is a Flask app that can be mounted to expose the remote-recording endpoint. """ | ||
| import json | ||
| """ | ||
| This file contains a Flask app that is mounted on /_appmap to expose the remote-recording endpoint | ||
| in a user's app. | ||
|
|
||
| from flask import Flask | ||
| It should only be imported if other code has already verified that Flask is available. | ||
| """ | ||
|
|
||
| from . import generation | ||
| from .recorder import Recorder | ||
| from .web_framework import AppmapMiddleware | ||
| from flask import Flask, Response | ||
|
|
||
| remote_recording = Flask(__name__) | ||
| from . import remote_recording | ||
|
|
||
| app = Flask(__name__) | ||
|
|
||
| @remote_recording.route("/record", methods=["GET"]) | ||
| def status(): | ||
| if not AppmapMiddleware.should_record(): | ||
| return "Appmap is disabled.", 404 | ||
|
|
||
| return {"enabled": Recorder.get_current().get_enabled()} | ||
| @app.route("/record", methods=["GET"]) | ||
| def status(): | ||
| body, rrstatus = remote_recording.status() | ||
| return Response(body, status=rrstatus, mimetype="application/json") | ||
|
|
||
|
|
||
| @remote_recording.route("/record", methods=["POST"]) | ||
| @app.route("/record", methods=["POST"]) | ||
| def start(): | ||
| r = Recorder.get_current() | ||
| if r.get_enabled(): | ||
| return "Recording is already in progress", 409 | ||
|
|
||
| r.start_recording() | ||
| return "", 200 | ||
| body, rrstatus = remote_recording.start() | ||
| return Response(body, status=rrstatus, mimetype="application/json") | ||
|
|
||
|
|
||
| @remote_recording.route("/record", methods=["DELETE"]) | ||
| @app.route("/record", methods=["DELETE"]) | ||
| def stop(): | ||
| r = Recorder.get_current() | ||
| if not r.get_enabled(): | ||
| return "No recording is in progress", 404 | ||
|
|
||
| r.stop_recording() | ||
|
|
||
| return json.loads(generation.dump(r)) | ||
| body, rrstatus = remote_recording.stop() | ||
| return Response(body, status=rrstatus, mimetype="application/json") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| """ remote_recording is a Flask app that can be mounted to expose the remote-recording endpoint. """ | ||
| import json | ||
| from threading import Lock | ||
|
|
||
| from . import generation | ||
| from .detect_enabled import DetectEnabled | ||
| from .recorder import Recorder | ||
|
|
||
| # pylint: disable=global-statement | ||
| _enabled_lock = Lock() | ||
| _enabled = False | ||
|
|
||
|
|
||
| def status(): | ||
| if not DetectEnabled.should_enable("remote"): | ||
| return "Appmap is disabled.", 404 | ||
|
|
||
| with _enabled_lock: | ||
| return json.dumps({"enabled": _enabled}), 200 | ||
|
|
||
|
|
||
| def start(): | ||
| global _enabled | ||
| with _enabled_lock: | ||
| if _enabled: | ||
| return "Recording is already in progress", 409 | ||
|
|
||
| Recorder.new_global().start_recording() | ||
| _enabled = True | ||
| return "", 200 | ||
|
|
||
|
|
||
| def stop(): | ||
| global _enabled | ||
| with _enabled_lock: | ||
| if not _enabled: | ||
| return "No recording is in progress", 404 | ||
| r = Recorder.get_global() | ||
| r.stop_recording() | ||
| _enabled = False | ||
| return generation.dump(r), 200 | ||
|
|
||
|
|
||
| def initialize(): | ||
| global _enabled | ||
| _enabled = False |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the interference of remote requests with remote recording?
Was part of it a race condition over event ids? If event ids are unique per thread then why put them behind a lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were three two aspects to the interference:
I fixed the first by tracking the state of remote recording with a
ContextVarwith a boolean value. I also simplified the code somewhat by separating it from the middleware that captureshttp_serverevents.I fixed the second by having the next event id be global (it's a class variable of
Recorder, which is why there's locking around updating it). Careful management of resetting it meant that no tests need to change. That management probably also means it's a bit fragile, but perhaps that an issue for another time.One thing that might be surprising about this PR: there aren't any updates for the expected AppMaps. This is actually because of the problem identified with AppMap correctness: the only route in the test Flask app is implemented as a function in module , so it doesn't show up in the recordings.
WDYT, does it seem worth adding a test that show enabling both kinds of recording results in a proper AppMap?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
I think test_record_request_appmap_enabled_requests_enabled_and_remote collects both kinds of recording results. I wonder if at least one of the many threads there contains data collected for remote recording. Maybe all it needs is an assert or two in record_request_thread.
Edit: after trying locally (by not deleting requests appmaps) I only see
"name":"record_requests". I don't see"name":"record_remote". This test may be hard to write. It's strange there's no appmap on my local disk for remote.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event ids are not unique per-thread. They're global, so the lock is required.
I'll push a
fixupcommit with the other changes shortly.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work for Django for me:
The
_appmap/recorddoesn't exist.It doesn't work for Flask either:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try running it like this:
Setting
PYTHONPATHhere simulates what happens when the package is really installed.