Send messages to docs.rs after crate actions#14188
Conversation
Rather than special casing the CDN log queue configuration, we make the configuration shared so we can reuse it for the docs.rs queue configuration. One of `config::queue` and `sqs` ends up needing to know about the other. Since `config` modules are generally simple, this has been implemented in `sqs`, which means that it is no longer self-contained, but does mean that we follow the same general pattern as other service clients.
As with the other trait methods, this is a thin wrapper around `aws_sdk_sqs`.
I have some mixed feelings on the utility of these tests, since they're really just validating that the very thin `SqsQueueImpl` methods are passing the right things between the caller and `aws_sdk_sqs`, but :shrug:.
This will be used to build the appropriate SQS message to docs.rs when set.
| (**self).receive_messages(max_messages).await | ||
| } | ||
|
|
||
| async fn send_message(&self, message: String) -> anyhow::Result<SendMessageOutput> { |
There was a problem hiding this comment.
Nit: As of this commit, the SqsQueue trait doesn't have the send_message method on it. That is, if you check out this commit, I don't think it will build.
There was a problem hiding this comment.
yeah, I think this was meant to be introduced two commits later in the "Add SqsQueue::send_message" commit
| diesel-async = { version = "=0.9.2", features = ["async-connection-wrapper", "deadpool", "postgres"] } | ||
| diesel_full_text_search = "=2.3.0" | ||
| diesel_migrations = { version = "=2.3.2", features = ["postgres"] } | ||
| docs_rs_crates_io = { git = "https://github.com/rust-lang/docs.rs.git", rev = "1604301c72411fbc63082434295fa45036d982b7", version = "0.1.0" } |
There was a problem hiding this comment.
just pointing out for other reviewers that the docs_rs_crates_io crate and this rev are from rust-lang/docs.rs#3357
There was a problem hiding this comment.
and I assume we won't rely on a git dependency once we merge this, right? 😅
|
🤷🏻♀️ seems fine to me |
| (**self).receive_messages(max_messages).await | ||
| } | ||
|
|
||
| async fn send_message(&self, message: String) -> anyhow::Result<SendMessageOutput> { |
There was a problem hiding this comment.
yeah, I think this was meant to be introduced two commits later in the "Add SqsQueue::send_message" commit
| diesel-async = { version = "=0.9.2", features = ["async-connection-wrapper", "deadpool", "postgres"] } | ||
| diesel_full_text_search = "=2.3.0" | ||
| diesel_migrations = { version = "=2.3.2", features = ["postgres"] } | ||
| docs_rs_crates_io = { git = "https://github.com/rust-lang/docs.rs.git", rev = "1604301c72411fbc63082434295fa45036d982b7", version = "0.1.0" } |
There was a problem hiding this comment.
and I assume we won't rely on a git dependency once we merge this, right? 😅
| /// The triggering event that led to the sync. | ||
| /// | ||
| /// Once the index has been synced, this event will be sent to docs.rs via | ||
| /// SQS. | ||
| #[serde(default)] | ||
| trigger: Option<Trigger>, |
There was a problem hiding this comment.
unfortunately I think this defeats the purposed of the const DEDUPLICATED: bool = true; below. we explicitly introduced the deduplication to avoid having to sync every update in isolation to the indexes because it was causing performance problems. if we include the triggers then either we don't deduplicate anymore, or we keep deduplicating but docs.rs might miss deduplicated events, or both.
it might also block us from potentially batching index updates in the future for related reasons.
does docs.rs really need this information? we can easily give them the added/updated/deleted information, but the yanked/unyanked/version-deleted would be slightly more complicated since we might combine multiple of these in a single commit.
|
|
||
| let crate_name = self.krate.clone(); | ||
| let mut conn = env.deadpool.get().await?; | ||
| let queue = env.docs_rs_queue.clone(); |
There was a problem hiding this comment.
we should probably make this queue optional, so that it does not break the publishing flow when developing locally without having a queue configured. the default-to-mock behavior for the other queue was okayish in the past because the corresponding background job that used the queue had to be triggered manually anyway, but this one is triggered by the regular publish flow and yank/unyank. this might involve changing QueueConfig::from_env() to return Result<Option<Self>> or something like that.
| // TODO: is this alertable? How can we notify docs.rs that rebuilds | ||
| // may be required? | ||
| error!( | ||
| krate.name = &self.krate, | ||
| trigger = ?self.trigger, | ||
| error = %e, | ||
| "error notifying docs.rs", | ||
| ); |
There was a problem hiding this comment.
you could do something like warn!(target: "docs_rs_queue", ...) which makes it easy to filter on in Datadog
I'd also recommend to not use structured logging too much unless you plan on filtering by these fields, and including the fields in the log message too, so that not every log row on Datadog reads "error notifying docs.rs" without any context. something like "Failed to notify docs.rs about {trigger} update on {krate} crate: {error}" is much easier to comprehend at a glance without having to dig into the details of every log event (see https://github.com/rust-lang/crates.io/blob/main/docs/LOGGING.md) :)
4 participants
This is a sketch of how we might close #14111 by sending messages to docs.rs. It is best reviewed commit-by-commit.1
I do not expect to merge this in its current form; this draft PR is primarily to gather feedback.
How it works
Very broadly, we need to do the following:
CdnLogQueueConfigto be generic so we can reuse it for the docs.rs queue.SqsQueue.SyncToGitIndexjob to include the event that triggered the sync2, which can then be turned into a message.Open questions
I'm not particularly set on this particular implementation. Things I'd like feedback on include:
SyncToGitIndexjob at all?SyncToGitIndex, or should this be decoupled from the Git index sync altogether?3SqsQueueImpltests actually useful?Other prerequisites and testing
This depends on rust-lang/simpleinfra#1099. Once that's merged, we could deploy this (or whatever comes out of this) to staging and start testing with a stub consumer to ensure that we get the messages we expect to see.
I'm unsure if the docs.rs team wants to actually publish the
docs_rs_crates_iocrate, but if they do, then that would also need to happen.Footnotes
I expect that, in practice, we might want to split the SQS commits into a separate PR since they're technically a separate refactor. ↩
There are still cases where we won't have a trigger that's relevant to docs.rs, most notably manual syncs to the index initiated via
crates-admin. ↩The main motivator behind tying this to the Git index sync is that docs.rs uses the Git index to pull crate metadata, so having the message only be sent after the Git index has already been updated means that docs.rs doesn't need to care about whether the update has occurred. ↩
In particular, I don't love how big that function has now gotten with the usage of
MockSqsQueue. ↩