-
Notifications
You must be signed in to change notification settings - Fork 76
Add /feedback endpoint #74
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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,153 @@ | ||
| """Handler for REST API call to provide info.""" | ||
|
|
||
| import logging | ||
| from typing import Any | ||
| from pathlib import Path | ||
| import json | ||
| from datetime import datetime, UTC | ||
|
|
||
| from fastapi import APIRouter, Request, HTTPException, Depends, status | ||
|
|
||
| from configuration import configuration | ||
| from models.responses import FeedbackResponse, StatusResponse | ||
| from models.requests import FeedbackRequest | ||
| from utils.suid import get_suid | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| router = APIRouter(prefix="/feedback", tags=["feedback"]) | ||
|
|
||
| # Response for the feedback endpoint | ||
| feedback_response: dict[int | str, dict[str, Any]] = { | ||
| 200: {"response": "Feedback received and stored"}, | ||
| } | ||
|
|
||
|
|
||
| def is_feedback_enabled() -> bool: | ||
| """Check if feedback is enabled. | ||
|
|
||
| Returns: | ||
| bool: True if feedback is enabled, False otherwise. | ||
| """ | ||
| return not configuration.user_data_collection_configuration.feedback_disabled | ||
|
|
||
|
|
||
| # TODO(lucasagomes): implement this function to retrieve user ID from auth | ||
| def retrieve_user_id(auth: Any) -> str: # pylint: disable=unused-argument | ||
| """Retrieve the user ID from the authentication handler. | ||
|
|
||
| Args: | ||
| auth: The Authentication handler (FastAPI Depends) that will | ||
| handle authentication Logic. | ||
|
|
||
| Returns: | ||
| str: The user ID. | ||
| """ | ||
| return "user_id_placeholder" | ||
|
|
||
|
|
||
| # TODO(lucasagomes): implement this function to handle authentication | ||
| async def auth_dependency(_request: Request) -> bool: | ||
| """Authenticate dependency to ensure the user is authenticated. | ||
|
|
||
| Args: | ||
| request (Request): The FastAPI request object. | ||
|
|
||
| Raises: | ||
| HTTPException: If the user is not authenticated. | ||
| """ | ||
| return True | ||
|
|
||
|
|
||
| async def assert_feedback_enabled(_request: Request) -> None: | ||
| """Check if feedback is enabled. | ||
|
|
||
| Args: | ||
| request (Request): The FastAPI request object. | ||
|
|
||
| Raises: | ||
| HTTPException: If feedback is disabled. | ||
| """ | ||
| feedback_enabled = is_feedback_enabled() | ||
| if not feedback_enabled: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="Forbidden: Feedback is disabled", | ||
| ) | ||
|
|
||
|
|
||
| @router.post("", responses=feedback_response) | ||
| def feedback_endpoint_handler( | ||
| _request: Request, | ||
| feedback_request: FeedbackRequest, | ||
| _ensure_feedback_enabled: Any = Depends(assert_feedback_enabled), | ||
| auth: Any = Depends(auth_dependency), | ||
| ) -> FeedbackResponse: | ||
| """Handle feedback requests. | ||
|
|
||
| Args: | ||
| feedback_request: The request containing feedback information. | ||
| ensure_feedback_enabled: The feedback handler (FastAPI Depends) that | ||
| will handle feedback status checks. | ||
| auth: The Authentication handler (FastAPI Depends) that will | ||
| handle authentication Logic. | ||
|
|
||
| Returns: | ||
| Response indicating the status of the feedback storage request. | ||
| """ | ||
| logger.debug("Feedback received %s", str(feedback_request)) | ||
|
|
||
| user_id = retrieve_user_id(auth) | ||
| try: | ||
| store_feedback(user_id, feedback_request.model_dump(exclude={"model_config"})) | ||
| except Exception as e: | ||
| logger.error("Error storing user feedback: %s", e) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail={ | ||
| "response": "Error storing user feedback", | ||
| "cause": str(e), | ||
| }, | ||
| ) from e | ||
|
|
||
| return FeedbackResponse(response="feedback received") | ||
|
|
||
|
|
||
| def store_feedback(user_id: str, feedback: dict) -> None: | ||
| """Store feedback in the local filesystem. | ||
|
|
||
| Args: | ||
| user_id: The user ID (UUID). | ||
| feedback: The feedback to store. | ||
| """ | ||
| logger.debug("Storing feedback for user %s", user_id) | ||
| # Creates storage path only if it doesn't exist. The `exist_ok=True` prevents | ||
| # race conditions in case of multiple server instances trying to set up storage | ||
| # at the same location. | ||
| storage_path = Path( | ||
| configuration.user_data_collection_configuration.feedback_storage or "" | ||
| ) | ||
| storage_path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| current_time = str(datetime.now(UTC)) | ||
| data_to_store = {"user_id": user_id, "timestamp": current_time, **feedback} | ||
|
|
||
| # stores feedback in a file under unique uuid | ||
| feedback_file_path = storage_path / f"{get_suid()}.json" | ||
| with open(feedback_file_path, "w", encoding="utf-8") as feedback_file: | ||
| json.dump(data_to_store, feedback_file) | ||
|
|
||
| logger.info("Feedback stored sucessfully at %s", feedback_file_path) | ||
|
|
||
|
|
||
| @router.get("/status") | ||
| def feedback_status() -> StatusResponse: | ||
| """Handle feedback status requests. | ||
|
|
||
| Returns: | ||
| Response indicating the status of the feedback. | ||
| """ | ||
| logger.debug("Feedback status requested") | ||
| feedback_status_enabled = is_feedback_enabled() | ||
| return StatusResponse( | ||
| functionality="feedback", status={"enabled": feedback_status_enabled} | ||
| ) | ||
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
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.
thought: I'm thinking about whether it would make sense to set some upper limit on the length of the user's feedback. This would be to prevent resource exhaustion if some malicious party decides to test out the endpoint.
question: Is the plan to store the feedback later somewhere in a DB and not in a plaintext file?
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.
True, we can think of that as well. For now, I'm just mimicking what was present in the previous service.
Regarding the question, looking at the data collection [0], it does seem like it still sends it in plain-text. It's just zipped and sent to console.redhat.com. May be something we want to change too.
But I think these things can be added on top of this work. For now, keeping it compatible to what it was before seems fine IMHO.
[0] https://github.com/road-core/service/tree/643164b47ea1cf9c91bd5a1ce3cadbed1fb3faa4/ols/user_data_collection
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.
Yes, definitely I think this is a good start! 👍 I was just curious.
Thanks for the link, it makes it a bit clearer to me. I do not know much about the feedback part.
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.
basically yes, the plan is to use DB (probably sqlite as it's used a lot in llama-stack itself). The JSON as files solution was easier to implement in openshift scenario, but personally I don't like it very much TBH :)
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.
@lpiwowar +1 for having config to limit feedback. Would you like to create a ticket for it? Or if you don't want to, I can take it, whatever
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.
If the format would change, also the pipelines in RH network will need changes... But doable, of course ;)
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.
@tisnik I can take a look at the limiting of the feedback. I'll create a ticket:).
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.
deal @lpiwowar :) TYVM
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.
Ticket created:), @umago it is on our board as well. It's a small ticket, but I'm just being transparent. I guess we are still figuring out how to best collaborate together.
Yes:), I guess this change would be a bit bigger. I guess if it works, then it is fine for now (?). Maybe something we might want to take later?