🪟 🐛 Fix messages and improve connector log #17344
Conversation
github-actions
bot
added
area/platform
| @@ -68,12 +68,12 @@ const MainInfo: React.FC<MainInfoProps> = ({ job, attempts = [], isOpen, onExpan | |||
| } else if (isPartialSuccess) { | |||
| status = "partialSuccess"; | |||
| } else { | |||
| return <FormattedMessage id="sources.jobStatus.unknown" />; | |||
| return <FormattedMessage id="jobs.jobStatus.unknown" />; | |||
There was a problem hiding this comment.
ℹ️ I've renamed this prefix to jobs, since this can show for source, connections, destinations, so I felt jobs might be a better prefix.
| "jobs.jobStatus.check_connection_source.succeeded": "Configuration check finished", | ||
| "jobs.jobStatus.check_connection_destination.failed": "Configuration check failed", | ||
| "jobs.jobStatus.check_connection_destination.succeeded": "Configuration check finished", | ||
| "jobs.jobStatus.discover_schema.succeeded": "Discovering schema succeeded", | ||
| "jobs.jobStatus.discover_schema.failed": "Discovering schema failed", |
There was a problem hiding this comment.
ℹ️ Since those types of jobs are Synchronous jobs, they can only result in success or failed and not in any of the other states.
| "jobs.jobStatus.sync.cancelled": "Sync Cancelled", | ||
| "jobs.jobStatus.sync.partialSuccess": "Sync Partial Success", | ||
| "jobs.jobStatus.check_connection_source.failed": "Configuration check failed", | ||
| "jobs.jobStatus.check_connection_source.succeeded": "Configuration check finished", |
There was a problem hiding this comment.
ℹ️ I tried to avoid calling it "succeeded" here for the connector check jobs, since the actual check inside that job might still determined the connection would fail (as shown in the screenshot in the PR description). Thus I felt "finished" would be the better term for those two types of jobs.
timroes
changed the title
| "jobs.jobStatus.check_connection_source.failed": "Configuration check failed", | ||
| "jobs.jobStatus.check_connection_source.succeeded": "Configuration check finished", | ||
| "jobs.jobStatus.check_connection_destination.failed": "Configuration check failed", | ||
| "jobs.jobStatus.check_connection_destination.succeeded": "Configuration check finished", |
There was a problem hiding this comment.
| "jobs.jobStatus.check_connection_source.failed": "Configuration check failed", | |
| "jobs.jobStatus.check_connection_source.succeeded": "Configuration check finished", | |
| "jobs.jobStatus.check_connection_destination.failed": "Configuration check failed", | |
| "jobs.jobStatus.check_connection_destination.succeeded": "Configuration check finished", | |
| "jobs.jobStatus.check_connection_source.failed": "Connection check failed", | |
| "jobs.jobStatus.check_connection_source.succeeded": "Connection check finished", | |
| "jobs.jobStatus.check_connection_destination.failed": "Connection check failed", | |
| "jobs.jobStatus.check_connection_destination.succeeded": "Connection check finished", |
I think these should be Connection check ... instead of Configuration check .... It would be confusing if this said Configuration check succeeded but then part of the configuration was wrong, e.g. a wrong pokemon name
There was a problem hiding this comment.
The reason I tried to avoid "Connection" here was that the we also have "Connections" as a concept in the application, and I found it a bit confusing saying that the "Connection (check) failed", while the user is just setting up connectors. Also not all connectors necessarily do a "connection check", e.g. the pokeapi wouldn't do. So I feel what really happens is that the "Connector configuration is validated", and tried to phrase that.
Regarding your point about "Configuration check succeeded" but the actual configuration being broken, that's exactly why I avoided the word "Succeeded" here, but said "Configuration check finished" in the success case, so it's not being confused with the configuration being actual correct.
Given that thoughts, what would be your preferred way for phrasing here? (I have no strong feelings either way so happy having you make the call on that)
There was a problem hiding this comment.
Ah okay, I wasn't sure if using Configuration instead of Connection here was intentional, but I think your justification makes sense, so I think this is fine. I agree that using finished instead of succeeded for that case makes sense due to the issue I pointed out
| {/* Show the job log only if advanced mode is turned on or the actual job failed (not the check inside the job) */} | ||
| {job && (advancedMode || !job.succeeded) && <JobItem job={job} />} |
There was a problem hiding this comment.
Would it make any sense to present the "check connection" job as "Failed" if the check inside the job was not successful?
To me, a "check connection" job is "unsuccessful" if either
- The job itself failed to complete for some reason (e.g. could not connect to temporal), or
- The check that is performed in the job failed (since that is the whole point of the check connection job)
I think it is useful to differentiate between these cases by having different failure messages/reasons shown, but I still consider both situations as a "failed check". So I feel that we should only show the "check connection" job as "successful" if it ran to completion and the check itself was successful. What do you think?
There was a problem hiding this comment.
Given that we hide it by default now from the UI at all, I feel we should keep it as successful even if the actual result of the check was negative (which we still show as negative, but the user won't see the successful Job run anymore unless they are in advanced mode). Given that it's a technical detail in advanced mode, I feel more inclined to not mess with the actual job status, and have that represent what happened to the actual technical job under the hood instead of trying to rewrite this. Also in case of the job being successful but the configuration check failed the logs won't have any error inside, which makes it a bit confusing if we'd show the job now as failed, but only have "successful" logs insight.
There was a problem hiding this comment.
Hmm okay I think I'm convinced that it's fine to leave it as is then. My only other suggestion is that maybe we could add a note or a tooltip somewhere in the successful job case that explains that even though the job was successful, that doesn't necessarily mean the actual check was successful
There was a problem hiding this comment.
Had one more small suggestion about adding a note/tooltip for the successful job, but I don't feel super strongly about that so I'll leave that up to you if you want to do anything like that.
I tested this out locally and it works as expected for the poke API. I think the Configuration check finished label feels pretty clear to me that it is just indicating that the configuration check ran to completion, so an extra note/tooltip may not be very necessary.
| {/* Show the job log only if advanced mode is turned on or the actual job failed (not the check inside the job) */} | ||
| {job && (advancedMode || !job.succeeded) && <JobItem job={job} />} |
There was a problem hiding this comment.
Hmm okay I think I'm convinced that it's fine to leave it as is then. My only other suggestion is that maybe we could add a note or a tooltip somewhere in the successful job case that explains that even though the job was successful, that doesn't necessarily mean the actual check was successful
| "jobs.jobStatus.check_connection_source.failed": "Configuration check failed", | ||
| "jobs.jobStatus.check_connection_source.succeeded": "Configuration check finished", | ||
| "jobs.jobStatus.check_connection_destination.failed": "Configuration check failed", | ||
| "jobs.jobStatus.check_connection_destination.succeeded": "Configuration check finished", |
There was a problem hiding this comment.
Ah okay, I wasn't sure if using Configuration instead of Connection here was intentional, but I think your justification makes sense, so I think this is fine. I agree that using finished instead of succeeded for that case makes sense due to the issue I pointed out
* Fix messages and improve connector log * Remove deprecated strings
What
This fixes some missing messages for the
JobItemcomponent as well as putting the logs after a connector check behind the advanced mode toggle (unless they failed).So far after a connector check we'd always show the log of the job, even if the job itself succeeded. This leads to the following confusing scenario for users:
The actual job to check if the connection is valid ran successfully (thus the log itself is green and successful), but the test that ran in that job determined that the actual data is wrong (i.e. wrong pokemon name in this case) and thus the actual check is failed.
As of this PR we're now no longer showing this log (which most of the time anyway doesn't have any useful information) unless advanced mode is on, or the actual job that should execute the connector check failed (e.g. because we had issues communicating with Temporal), in which case that log might contain useful information for debugging.