Skip to content

fix(general): clean up typos and code #62

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.

Sign up for GitHub

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

Merged
pankajagrawal16 merged 4 commits into from
Sep 1, 2020
Merged

fix(general): clean up typos and code #62

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d30f14b
fix(general): clean up typos and code
michaelbrewer Aug 31, 2020
eb60c95
Merge branch 'master' into fix-cleanup
michaelbrewer Sep 1, 2020
a187340
fix: use Optional to simplify null handling
michaelbrewer Sep 1, 2020
c6d5227
refactor: revert null check code.
michaelbrewer Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/bug_report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ assignees: ''

> [How to enable debug mode](https://awslabs.github.io/aws-lambda-powertools-java/#debug-mode)**

```java
```text
# paste logs here
```
2 changes: 1 addition & 1 deletion docs/content/core/tracing.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public class App implements RequestHandler<APIGatewayProxyRequestEvent, APIGatew
Entity traceEntity = AWSXRay.getTraceEntity();

Thread anotherThread = new Thread(() -> withEntitySubsegment("inlineLog", traceEntity, subsegment -> {
// Business logic in sperate thread
// Business logic in separate thread
}));
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/software/amazon/lambda/powertools/core/internal/LambdaHandlerProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.aspectj.lang.ProceedingJoinPoint;

public final class LambdaHandlerProcessor {
private static String SERVICE_NAME = null != System.getenv("POWERTOOLS_SERVICE_NAME")
private static String SERVICE_NAME = System.getenv("POWERTOOLS_SERVICE_NAME") != null
Copy link
Contributor

@pankajagrawal16 pankajagrawal16 Aug 31, 2020

Choose a reason for hiding this comment

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

In order to keep it consistent with how we will like to check for null, I will prefer if we keep constant(null) to the left.

Copy link
Contributor Author

@michaelbrewer michaelbrewer Sep 1, 2020

Choose a reason for hiding this comment

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

Ok sure, i can revert this, just confusing to read left to right.

Maybe this would be cleaner and work with java8

private static String SERVICE_NAME = Optional.ofNullable(System.getenv("POWERTOOLS_SERVICE_NAME")).orElse("service_undefined")

Copy link
Contributor Author

@michaelbrewer michaelbrewer Sep 1, 2020

Choose a reason for hiding this comment

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

@pankajagrawal16 ☝️

Copy link
Contributor Author

@michaelbrewer michaelbrewer Sep 1, 2020

Choose a reason for hiding this comment

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

@pankajagrawal16 SERVICE_NAME and IS_COLD_START look like constants, but are actually mutable due to the unit tests requiring this to be mutable. Maybe we should change either the unit tests or these misleading names.

Copy link
Contributor

@pankajagrawal16 pankajagrawal16 Sep 1, 2020

Choose a reason for hiding this comment

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

Ok sure, i can revert this, just confusing to read left to right.

Maybe this would be cleaner and work with java8

private static String SERVICE_NAME = Optional.ofNullable(System.getenv("POWERTOOLS_SERVICE_NAME")).orElse("service_undefined")

We have intentionally avoided use of optional here considering its a simple null check which belongs to an internal class.

I am a big fan of functional interfaces in java myself and have to always find a balance when I would like to use it.

Prefer using optional on public API that is public to let users ability to write functional code and not worry about nulls.

But these are just personal preferences too. Considering this being a internal class, I will still be happy to leave it the way it was. Thanks for the input. 🙂

Copy link
Contributor Author

@michaelbrewer michaelbrewer Sep 1, 2020

Choose a reason for hiding this comment

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

@pankajagrawal16 - ok i reverted back to the original code.

? System.getenv("POWERTOOLS_SERVICE_NAME") : "service_undefined";
private static Boolean IS_COLD_START = null;

Expand Down
25 changes: 10 additions & 15 deletions powertools-logging/src/main/java/org/apache/logging/log4j/core/layout/LambdaJsonLayout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,10 @@
*/
package org.apache.logging.log4j.core.layout;

import java.io.IOException;
import java.io.Writer;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;

import com.fasterxml.jackson.annotation.JsonAnyGetter;
import com.fasterxml.jackson.annotation.JsonGetter;
import com.fasterxml.jackson.annotation.JsonRootName;
import com.fasterxml.jackson.annotation.JsonUnwrapped;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.logging.log4j.core.Layout;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.config.Configuration;
Expand All @@ -36,11 +25,20 @@
import org.apache.logging.log4j.core.config.plugins.Plugin;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
import org.apache.logging.log4j.core.jackson.Log4jJsonObjectMapper;
import org.apache.logging.log4j.core.jackson.XmlConstants;
import org.apache.logging.log4j.core.util.KeyValuePair;
import org.apache.logging.log4j.util.Strings;

import java.io.IOException;
import java.io.Writer;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;

import static java.time.Instant.ofEpochMilli;
import static java.time.format.DateTimeFormatter.ISO_ZONED_DATE_TIME;

Expand All @@ -52,8 +50,6 @@ public class LambdaJsonLayout extends AbstractJacksonLayout {

static final String CONTENT_TYPE = "application/json";

private final ObjectMapper objectMapper;

public static class Builder<B extends Builder<B>> extends AbstractJacksonLayout.Builder<B>
implements org.apache.logging.log4j.core.util.Builder<LambdaJsonLayout> {

Expand Down Expand Up @@ -113,7 +109,6 @@ private LambdaJsonLayout(final Configuration config, final boolean locationInfo,
PatternLayout.newSerializerBuilder().setConfiguration(config).setPattern(footerPattern).setDefaultPattern(DEFAULT_FOOTER).build(),
includeNullDelimiter,
additionalFields);
this.objectMapper = new Log4jJsonObjectMapper(encodeThreadContextAsList, includeStacktrace, stacktraceAsString, objectMessageAsJsonObject);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ...ls-logging/src/main/java/software/amazon/lambda/powertools/logging/PowertoolsLogging.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
* <p>By default {@code PowertoolsLogging} will not log the event which has trigger the invoke of the Lambda function.
* This can be enabled using {@code @PowertoolsLogging(logEvent = true)}.</p>
*
* <p>By default {@code PowertoolsLogging} all debug loggs will follow log4j2 configuration unless configured via
* <p>By default {@code PowertoolsLogging} all debug logs will follow log4j2 configuration unless configured via
* POWERTOOLS_LOGGER_SAMPLE_RATE environment variable {@code @PowertoolsLogging(samplingRate = <0.0-1.0>)}.</p>
*
* <p>To append additional keys to each log entry you can use {@link PowertoolsLogger#appendKey(String, String)}</p>
Expand Down
1 change: 0 additions & 1 deletion ...src/test/java/software/amazon/lambda/powertools/sqs/handlers/LambdaHandlerApiGateway.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import com.amazonaws.services.lambda.runtime.Context;
import com.amazonaws.services.lambda.runtime.RequestHandler;
import com.amazonaws.services.lambda.runtime.events.APIGatewayProxyRequestEvent;
import com.amazonaws.services.lambda.runtime.events.APIGatewayProxyResponseEvent;
import software.amazon.lambda.powertools.sqs.LargeMessageHandler;

public class LambdaHandlerApiGateway implements RequestHandler<APIGatewayProxyRequestEvent, String> {
Expand Down
2 changes: 1 addition & 1 deletion ...ls-tracing/src/main/java/software/amazon/lambda/powertools/tracing/PowertoolsTracing.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
* <p>By default {@code PowertoolsTracing} will capture responses and add them
* to a sub segment named after the method.</p>
*
* <p>To disable this functionality you can specify {@code @PowertoolsTracing( captureRespones = false)}</p>
* <p>To disable this functionality you can specify {@code @PowertoolsTracing( captureResponse = false)}</p>
*
* <p>By default {@code PowertoolsTracing} will capture errors and add them
* to a sub segment named after the method.</p>
Expand Down