-
Notifications
You must be signed in to change notification settings - Fork 1
Hea 752/dagster graph ql api is intermittently failing with a protocol error when accessed via the revproxy django view #176
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
air-github-actions-user
merged 12 commits into
main
from
HEA-752/Dagster-GraphQL-API-is-intermittently-failing-with-a-ProtocolError-when-accessed-via-the-revproxy-Django-view
Oct 30, 2025
Merged
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9781bd8
Add DagsterWebSocketProxyConsumer and update the routing to support w…
girumb dffc109
Remove non required settings see HEA-752
girumb 5720990
Merge branch 'main' into HEA-752/Dagster-GraphQL-API-is-intermittentl…
girumb 899d76c
Merge branch 'main' into HEA-752/Dagster-GraphQL-API-is-intermittentl…
rhunwicks bd69551
Add log supress entry for removing chatty websocket ping, pong logs s…
girumb 466a603
Override common app ready to patch urllib3 pool see HEA-752
girumb f7658cc
Merge branch 'HEA-752/Dagster-GraphQL-API-is-intermittently-failing-w…
girumb e2c246c
Merge branch 'main' into HEA-752/Dagster-GraphQL-API-is-intermittentl…
girumb bcd480c
Merge branch 'main' into HEA-752/Dagster-GraphQL-API-is-intermittentl…
girumb e5cef4c
Add logging filters, apply the filters and address PR feedback see HE…
girumb a7c4022
Address PR feedback see HEA-752
girumb 0e74c37
Merge branch 'main' into HEA-752/Dagster-GraphQL-API-is-intermittentl…
girumb 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| import asyncio | ||
| import logging | ||
| import os | ||
|
|
||
| import websockets | ||
| from channels.exceptions import DenyConnection | ||
| from channels.generic.websocket import AsyncWebsocketConsumer | ||
| from django.contrib.auth.models import AnonymousUser | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class DagsterWebSocketProxyConsumer(AsyncWebsocketConsumer): | ||
|
|
||
| async def connect(self): | ||
| logger.info(f"WebSocket connection attempt: {self.scope['path']}") | ||
|
|
||
| # Authentication check | ||
| if isinstance(self.scope["user"], AnonymousUser): | ||
| logger.error("Authentication required") | ||
| raise DenyConnection("Authentication required") | ||
|
|
||
| if not self.scope["user"].has_perm("common.access_dagster_ui"): | ||
| logger.error(f"User {self.scope['user'].username} lacks permission") | ||
| raise DenyConnection("Permission denied") | ||
|
|
||
| logger.info(f"User {self.scope['user'].username} authenticated") | ||
|
|
||
| # Build upstream URL | ||
| dagster_url = os.environ.get("DAGSTER_WEBSERVER_URL", "http://localhost:3000") | ||
| dagster_prefix = os.environ.get("DAGSTER_WEBSERVER_PREFIX", "") | ||
|
|
||
| path = self.scope["path"] | ||
| if path.startswith("/pipelines/"): | ||
| path = path[len("/pipelines/") :] | ||
|
|
||
| # Convert http to ws | ||
| if dagster_url.startswith("https://"): | ||
| ws_url = dagster_url.replace("https://", "wss://", 1) | ||
| else: | ||
| ws_url = dagster_url.replace("http://", "ws://", 1) | ||
|
|
||
| # Build target URL | ||
| if dagster_prefix: | ||
| target_url = f"{ws_url}/{dagster_prefix}/{path}" | ||
| else: | ||
| target_url = f"{ws_url}/{path}" | ||
|
|
||
| # Add query string | ||
| if self.scope.get("query_string"): | ||
| target_url += f"?{self.scope['query_string'].decode()}" | ||
|
|
||
| logger.info(f"Connecting to upstream: {target_url}") | ||
|
|
||
| # Get subprotocols from client | ||
| subprotocols = self.scope.get("subprotocols", []) | ||
|
|
||
| try: | ||
| self.websocket = await websockets.connect( | ||
| target_url, | ||
| max_size=2097152, | ||
| ping_interval=20, | ||
| subprotocols=subprotocols if subprotocols else None, | ||
| ) | ||
| logger.info("Connected to upstream") | ||
| except Exception as e: | ||
| logger.error(f"Failed to connect: {e}") | ||
| raise DenyConnection(f"Connection to upstream failed: {e}") | ||
|
|
||
| await self.accept(self.websocket.subprotocol) | ||
| logger.info(f"Client accepted with subprotocol: {self.websocket.subprotocol}") | ||
|
|
||
| self.consumer_task = asyncio.create_task(self.consume_from_upstream()) | ||
|
|
||
| async def disconnect(self, close_code): | ||
| logger.info(f"Disconnecting with code {close_code}") | ||
| if hasattr(self, "consumer_task"): | ||
| self.consumer_task.cancel() | ||
| try: | ||
| await self.consumer_task | ||
| except asyncio.CancelledError: | ||
| pass | ||
| if hasattr(self, "websocket"): | ||
| await self.websocket.close() | ||
|
|
||
| async def receive(self, text_data=None, bytes_data=None): | ||
| try: | ||
| await self.websocket.send(bytes_data or text_data) | ||
| except Exception as e: | ||
| logger.error(f"Error forwarding to upstream: {e}") | ||
| await self.close() | ||
|
|
||
| async def consume_from_upstream(self): | ||
| try: | ||
| async for message in self.websocket: | ||
| if isinstance(message, bytes): | ||
| await self.send(bytes_data=message) | ||
| else: | ||
| await self.send(text_data=message) | ||
| except asyncio.CancelledError: | ||
| pass | ||
| except Exception as e: | ||
| logger.error(f"Error consuming from upstream: {e}") | ||
| await self.close() |
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,8 @@ | ||
| from django.urls import re_path | ||
|
|
||
| from common.consumers import DagsterWebSocketProxyConsumer | ||
|
|
||
| websocket_urlpatterns = [ | ||
| # Route WebSocket connections for Dagster proxy | ||
| re_path(r"^pipelines/(?P<path>.*)$", DagsterWebSocketProxyConsumer.as_asgi()), | ||
| ] |
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 |
|---|---|---|
|
|
@@ -109,6 +109,7 @@ | |
| "rest_framework_gis", | ||
| "revproxy", | ||
| "corsheaders", | ||
| "channels", | ||
| ] | ||
| PROJECT_APPS = ["common", "metadata", "baseline"] | ||
| INSTALLED_APPS = EXTERNAL_APPS + PROJECT_APPS | ||
|
|
@@ -155,6 +156,8 @@ | |
| "SEARCH_PARAM": "search", | ||
| } | ||
|
|
||
| ASGI_APPLICATION = "hea.asgi.application" | ||
| CHANNEL_LAYERS = {"default": {"BACKEND": "channels.layers.InMemoryChannelLayer"}} | ||
|
|
||
| ########## CORS CONFIGURATION | ||
| # See: https://github.com/ottoyiu/django-cors-headers | ||
|
|
@@ -280,11 +283,11 @@ | |
| "django.security": {"handlers": ["console", "logfile"], "level": "ERROR", "propagate": False}, | ||
| "factory": {"handlers": ["console", "logfile"], "level": "INFO"}, | ||
| "faker": {"handlers": ["console", "logfile"], "level": "INFO"}, | ||
| "luigi": {"level": "INFO"}, | ||
| "luigi-interface": {"level": "INFO"}, | ||
| "urllib3": {"handlers": ["console", "logfile"], "level": "INFO", "propagate": False}, | ||
| "common.models": {"handlers": ["console", "logfile"], "level": "INFO", "propagate": False}, | ||
| "common.signals": {"handlers": ["console", "logfile"], "level": "INFO", "propagate": False}, | ||
| "uvicorn.error": {"handlers": ["console"], "level": "WARNING", "propagate": False}, | ||
|
||
| "uvicorn.access": {"handlers": ["console"], "level": "WARNING", "propagate": False}, | ||
| }, | ||
| # Keep root at DEBUG and use the `level` on the handler to control logging output, | ||
| # so that additional handlers can be used to get additional detail, e.g. `common.resources.LoggingResourceMixin` | ||
|
|
||
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
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.
This seems very non-standard.
My reading around this is that it's not an error, even if we would get better performance if we increased the pool max_size.
Do we know if the message is caused by the regular proxy or by the websockets one?
I am tempted to suppress the log message, but otherwise leave it all alone for now. Perhaps we start a new MR based on the original one, but without these commits.