Skip to content

feat(dcs): chat with folder backend#585

Merged
ehayes2000 merged 7 commits intomainfrom
chat-with-folder-backend
Dec 12, 2025
Merged

feat(dcs): chat with folder backend#585
ehayes2000 merged 7 commits intomainfrom
chat-with-folder-backend

Conversation

@ehayes2000
Copy link
Copy Markdown
Contributor

  • fetch folders
  • folder attachments
  • Read project tool

@ehayes2000 ehayes2000 requested a review from a team as a code owner December 12, 2025 04:28
@ehayes2000 ehayes2000 changed the title feat(dcs): chat with folder support feat(dcs): chat with folder backend Dec 12, 2025
Copy link
Copy Markdown
Member

@whutchinson98 whutchinson98 left a comment

Choose a reason for hiding this comment

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

due to using tracing json for datadog we actually lose the underlying error message when context is provided. Sometimes this is fine but most of the time we should actually have tracing instrument log the error for us and we don't want to lose the useful underlying error.

also, please prefer using tracing over eprintln and println as tracing is our standard as its configurable


serde_json::from_value(json)
.inspect_err(|err| eprintln!("jsonfail {:#?}", err))
.context("unexpected response")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove context

.context("failed to fetch json")?;

serde_json::from_value(json)
.inspect_err(|err| eprintln!("jsonfail {:#?}", err))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of inspect, just have tracing instrument err so it's automatically logged

make sure whatever is using document_storage_client has log level set to error

.context("failed to fetch head")?
.json()
.await
.context("failed to fetch json")?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no context

.external_request(reqwest::Method::GET, path.as_str(), jwt)
.send()
.await
.context("failed to fetch head")?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no context

.fetch_project(attachment.attachment_id.clone(), jwt.to_owned())
.content()
.await
.context("failed to fetch project")?
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

context

use super::DocumentStorageServiceClient;

impl DocumentStorageServiceClient {
#[tracing::instrument(skip(self))]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you please add err here to the instrument

@ehayes2000
Copy link
Copy Markdown
Contributor Author

due to using tracing json for datadog we actually lose the underlying error message when context is provided. Sometimes this is fine but most of the time we should actually have tracing instrument log the error for us and we don't want to lose the useful underlying error.

also, please prefer using tracing over eprintln and println as tracing is our standard as its configurable

the println was left in by mistake and has been removed. Good to know that context doesn't DD well. It's been removed.

@whutchinson98
Copy link
Copy Markdown
Member

due to using tracing json for datadog we actually lose the underlying error message when context is provided. Sometimes this is fine but most of the time we should actually have tracing instrument log the error for us and we don't want to lose the useful underlying error.
also, please prefer using tracing over eprintln and println as tracing is our standard as its configurable

the println was left in by mistake and has been removed. Good to know that context doesn't DD well. It's been removed.
it technically datadogs well, it's because for datadog we use the json transformation on tracing subscriber. that is the part that actually fails. if we didn't use json our logs would be multi-line in DD which would be very hard to reason about

@ehayes2000 ehayes2000 merged commit 0ded7f7 into main Dec 12, 2025
35 checks passed
@ehayes2000 ehayes2000 deleted the chat-with-folder-backend branch December 12, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants