Skip to content
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

JPERF-1106: Gather Apache2 logs #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pczuj
Copy link
Contributor

@pczuj pczuj commented May 11, 2023

No description provided.

@pczuj
Copy link
Contributor Author

pczuj commented May 11, 2023

This requires at least 4 changes:

)
val loadBalancerResultsSource = (provisionedLoadBalancer.loadBalancer as? DiagnosableLoadBalancer)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

First time used this as? syntax. Casts are usually bad, however given the circumstances of our current model I think this is a better solution than putting a new field into ProvisionedLoadBalancer and I'm also not certain if adding methods to LoadBalancer would be a good idea. Extending the interface and detecting it allows for wrapping of DiagnosableLoadBalancer implementations without losing the feature.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed casts are bad. See https://github.com/atlassian/aws-infrastructure/blob/release-2.29.0/src/main/kotlin/com/atlassian/performance/tools/awsinfrastructure/api/jira/DataCenterFormula.kt#L360:

if (loadBalancer is ApacheProxyLoadBalancer) listOf(loadBalancer::updateJiraConfiguration) else emptyList()

Fortunately, ApacheProxyLoadBalancer will match both is/as? casts, so it will function.
We need hooks API to avoid further tight coupling. If the apache logs are necessary, then we can merge the tech debt. But if not, then let's skip that part.

import com.atlassian.performance.tools.aws.api.StorageLocation
import com.atlassian.performance.tools.infrastructure.api.loadbalancer.LoadBalancer

interface DiagnosableLoadBalancer : LoadBalancer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name suggestions welcome

import com.atlassian.performance.tools.infrastructure.api.loadbalancer.LoadBalancer

interface DiagnosableLoadBalancer : LoadBalancer {
fun gatherDiagnostics(location: StorageLocation)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name suggestions welcome

import com.atlassian.performance.tools.aws.api.StorageLocation
import com.atlassian.performance.tools.infrastructure.api.loadbalancer.LoadBalancer

interface DiagnosableLoadBalancer : LoadBalancer {
Copy link
Contributor Author

@pczuj pczuj May 18, 2023

Choose a reason for hiding this comment

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

We could make that LoadBalancer-agnostic, however that would make the model more abstract and that possibly could make it harder to understand. Looking for thoughts on that.

@pczuj pczuj force-pushed the issue/JPERF-1106-gather-apache2-logs branch 2 times, most recently from 38c9b50 to 693a749 Compare May 18, 2023 22:03
@pczuj pczuj force-pushed the issue/JPERF-1106-gather-apache2-logs branch from 693a749 to 05c1ddd Compare May 19, 2023 10:32
@pczuj
Copy link
Contributor Author

pczuj commented May 19, 2023

I migrated the new tests to use AssertJ

@pczuj pczuj marked this pull request as ready for review May 19, 2023 10:57
@pczuj pczuj requested a review from a team as a code owner May 19, 2023 10:57
@pczuj
Copy link
Contributor Author

pczuj commented May 23, 2023

@dagguh do you see value in this change? If not we may want to decline it.

We used the logs in our internal experiment to diagnose problem and we already know that the problem is with Jira memory usage and not load balancer. Load balancer proxy error was only a symptom of product having CPU/IO problems.

@dagguh
Copy link
Member

dagguh commented May 25, 2023

Overall reuse of MeasurementSource is good. Now the branch needs more work due to breaking changes on master. I trust your call to continue or cancel this.

@pczuj pczuj force-pushed the issue/JPERF-1106-gather-apache2-logs branch from 05c1ddd to af09430 Compare May 27, 2023 18:37
@pczuj pczuj requested a review from ewefie May 27, 2023 18:39
@pczuj
Copy link
Contributor Author

pczuj commented Jun 5, 2023

This is ready for review again. I'm not planning to add integration test for the logs.

- Make `StartedNode` a `MeasurementSource`.

## Fixed
- Let every `MeasurementSource` inside `Jira` finish its gathering even if one of them fails. Fix [JPERF-1114].
Copy link
Member

Choose a reason for hiding this comment

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

Let's split those changes. One sounds good, while the other needs some further considerations.

)
val loadBalancerResultsSource = (provisionedLoadBalancer.loadBalancer as? DiagnosableLoadBalancer)
Copy link
Member

Choose a reason for hiding this comment

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

Indeed casts are bad. See https://github.com/atlassian/aws-infrastructure/blob/release-2.29.0/src/main/kotlin/com/atlassian/performance/tools/awsinfrastructure/api/jira/DataCenterFormula.kt#L360:

if (loadBalancer is ApacheProxyLoadBalancer) listOf(loadBalancer::updateJiraConfiguration) else emptyList()

Fortunately, ApacheProxyLoadBalancer will match both is/as? casts, so it will function.
We need hooks API to avoid further tight coupling. If the apache logs are necessary, then we can merge the tech debt. But if not, then let's skip that part.

@pczuj
Copy link
Contributor Author

pczuj commented Jun 14, 2023

If the apache logs are necessary, then we can merge the tech debt. But if not, then let's skip that part.

Not sure how to judge that.

We wanted to take a look at the appache logs when diagnosing a problem recently. We used JAR built from this code to do so, however didn't really find anything useful other than confirmation that the load balancer just times out on communication with the product.

We no longer have an immediate problem to diagnose, so that would mean that the logs are not necessary. Question is if we wouldn't waste the effort if the diagnostic need will repeat itself, yet the code for it won't be merged and e.g. will have conflicts with the current master.

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