Skip to content

update: added logs #142

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

Closed
wants to merge 6 commits into from
Closed

update: added logs #142

wants to merge 6 commits into from

Conversation

heemankv
Copy link
Contributor

@heemankv heemankv commented Oct 8, 2024

This PR resolves #137.

@heemankv heemankv requested a review from apoorvsadana October 8, 2024 12:26
@heemankv heemankv self-assigned this Oct 8, 2024
@coveralls
Copy link

coveralls commented Oct 8, 2024

Coverage Status

coverage: 64.36% (-0.5%) from 64.854%
when pulling 7014309 on update/logs-addition
into 705b119 on main.

@heemankv heemankv force-pushed the update/logs-addition branch from 5204c86 to 6631d93 Compare October 9, 2024 03:50
@heemankv heemankv force-pushed the update/logs-addition branch from f2b1278 to 54e83e6 Compare October 9, 2024 03:54
@heemankv heemankv linked an issue Oct 9, 2024 that may be closed by this pull request
@heemankv heemankv changed the title update: added logs for orchestrator update: added logs Oct 9, 2024
Copy link
Contributor Author

@heemankv heemankv left a comment

Choose a reason for hiding this comment

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

Done with Self Review

Copy link
Member

@Mohiiit Mohiiit left a comment

Choose a reason for hiding this comment

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

I have less context on code so maybe you would be able to make a better decision on whether to use info or debug on some places.

// Here in case of ethereum we are not publishing the state diff because we are doing it all
// together in update_state job. So we don't need to send the blob here.
Ok("NA".to_string())
}

async fn verify_inclusion(&self, _external_id: &str) -> Result<DaVerificationStatus> {
tracing::info!("Verifying inclusion on ethereum");
Copy link
Member

Choose a reason for hiding this comment

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

shall we add external id in the tracing as well?

@@ -49,6 +49,7 @@ impl DataStorage for AWSS3 {
let response = self.client.get_object().bucket(&self.bucket).key(key).send().await?;
let data_stream = response.body.collect().await.expect("Failed to convert body into AggregatedBytes.");
let data_bytes = data_stream.into_bytes();
tracing::info!("Got data from s3 bucket {:?}", self.bucket);
Copy link
Member

Choose a reason for hiding this comment

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

maybe add key as well? in case we have it wrong? wdyt

@@ -63,6 +64,7 @@ impl DataStorage for AWSS3 {
.send()
.await?;

tracing::info!("Put data to s3 bucket {:?}", self.bucket);
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@@ -137,6 +141,7 @@ impl Database for MongoDb {
}
};
self.update_job_optimistically(job, update).await?;
tracing::info!("DB: Updated job with id {:?} to status {:?}", job.id, new_status);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tracing::info!("DB: Updated job with id {:?} to status {:?}", job.id, new_status);
tracing::info!("DB: Updated status for job with id {:?} to status {:?}", job.id, new_status);

@@ -72,6 +71,7 @@ impl Job for DaJob {
internal_id: String,
metadata: HashMap<String, String>,
) -> Result<JobItem, JobError> {
tracing::debug!("DA JOB: Creating job with internal_id {:?}", internal_id);
Copy link
Member

Choose a reason for hiding this comment

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

I have less context, so wdyt?

Suggested change
tracing::debug!("DA JOB: Creating job with internal_id {:?}", internal_id);
tracing::info!("DA JOB: Creating job with internal_id {:?}", internal_id);

CairoJobStatus::INVALID => {
Ok(TaskStatus::Failed(format!("Task is invalid: {:?}", res.invalid_reason.unwrap_or_default())))
let invalid_reason = res.invalid_reason.unwrap_or_default();
tracing::error!("Sharp Service: Task is invalid: {:?}", invalid_reason.clone());
Copy link
Member

Choose a reason for hiding this comment

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

do we need clone here?

let program_output: Vec<U256> = vec_u8_32_to_vec_u256(program_output.as_slice())?;
let onchain_data_hash: U256 = slice_u8_to_u256(&onchain_data_hash)?;
let onchain_data_size = U256::from_be_bytes(onchain_data_size);
let tx_receipt =
self.core_contract_client.update_state(program_output, onchain_data_hash, onchain_data_size).await?;
tracing::info!("Ethereum Settlement Client: Updated state with calldata {:?}", tx_receipt.transaction_hash);
Copy link
Member

Choose a reason for hiding this comment

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

message not relevant, need to update this

Ok(SettlementVerificationStatus::Pending)
} else {
tracing::info!("Transaction {} is settled", tx_hash);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tracing::info!("Transaction {} is settled", tx_hash);
tracing::info!("Transaction {} is verified", tx_hash);


let tx_hash = Felt::from_hex(tx_hash)?;
loop {
let tx_receipt = self.account.provider().get_transaction_receipt(tx_hash).await?;
if tx_receipt.block.is_pending() {
retries += 1;
if retries > MAX_RETRIES_VERIFY_TX_FINALITY {
tracing::error!("Max retries exceeeded while waiting for tx {tx_hash} finality.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tracing::error!("Max retries exceeeded while waiting for tx {tx_hash} finality.");
tracing::error!("Max retries exceeeded while waiting for tx {:?} finality.", tx_hash);

return Err(eyre!("Max retries exceeeded while waiting for tx {tx_hash} finality."));
}
sleep(duration_to_wait_between_polling).await;
} else {
tracing::info!("Tx {tx_hash} has achieved finality");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tracing::info!("Tx {tx_hash} has achieved finality");
tracing::info!("Tx {:?} has achieved finality", tx_hash);

@apoorvsadana
Copy link
Contributor

Closing in favor of #147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add more debug and trace logs in code
4 participants