Skip to content

Refactor Elasticsearch metrics#1701

Merged
alexshtin merged 5 commits intotemporalio:masterfrom
alexshtin:feature/refactor-elasticsearch-metrics
Jul 3, 2021
Merged

Refactor Elasticsearch metrics#1701
alexshtin merged 5 commits intotemporalio:masterfrom
alexshtin:feature/refactor-elasticsearch-metrics

Conversation

@alexshtin
Copy link
Copy Markdown
Contributor

@alexshtin alexshtin commented Jul 2, 2021

What changed?
Refactor Elasticsearch related metrics. Renamed them to have elasticsearch prefix and arranged other definitions.
Also moved ack channel creation into processor.go.

Why?
Unify metrics.

How did you test it?
Modified existing and added new unit tests. Local canary run.

Potential risks
Few Elasticsearch metrics are renamed and dashboards need to be updated.

Is hotfix candidate?
No.

@alexshtin alexshtin requested review from a team, Ardagan and wxing1292 July 2, 2021 03:23
@alexshtin alexshtin force-pushed the feature/refactor-elasticsearch-metrics branch from c267034 to a6ae916 Compare July 2, 2021 20:21
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any possibility this channel is already acked or what if Done is called > 1 times

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

above code seem to have possibility to get the the channel > 1 times, so a.ackChInternal <- ack should be

select {
case a.ackChInternal <- ack:
default:
    // noop
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is just a refactoring. This code was there just wasn't extracted as a method. There are 2 calls to Done() in code above: normal case (success/error) or duplicate visibilityTaskKey. Both of them a guaranteed to be called only once by mapToAckChan and in case of duplicate the item get overwritten and never used again.

Comment on lines 202 to 204
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

which case will have visibilityTaskKey == ""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When extractVisibilityTaskKey wasn't able to extract it. It doesn't return error because it already logged it and reported metrics. So here I just ignore the item.

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.

I know everyone feels code should be self-documenting (and it should whenever possible) but personally I think that when an experienced reviewer familiar with the code has a question about what's going on, that's a good sign that a comment might be worthwhile. Just saying :)

Comment on lines 176 to 174
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe return an error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it is ok. It should never ever happend. Actually event removing ,ok part would be a better option and just let it panic with default go message.

@alexshtin alexshtin force-pushed the feature/refactor-elasticsearch-metrics branch from a6ae916 to d1406f1 Compare July 3, 2021 06:10
@alexshtin alexshtin merged commit cd10b35 into temporalio:master Jul 3, 2021
@alexshtin alexshtin deleted the feature/refactor-elasticsearch-metrics branch July 3, 2021 06:38
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.

4 participants