Fix charge span leak in the payment service

CHANGELOG.md

@@ -7,6 +7,11 @@ the release.
 
 ## Unreleased
 
+* [payment] Fix `charge` span lifecycle and exception attribution: wrap charge
+  logic in `try/catch/finally` to ensure the span is always ended, record
+  exceptions on the `charge` span where they originate, and remove duplicate
+  `recordException` from the gRPC handler
+  ([#3276](https://github.com/open-telemetry/opentelemetry-demo/pull/3276))
 * [recommendation] Fix `recommendationCacheFailure` feature flag by
   using `ListProducts` instead of `GetProduct`
   ([#3260](https://github.com/open-telemetry/opentelemetry-demo/pull/3260))

src/payment/charge.js

@@ -1,6 +1,6 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
-const { context, propagation, trace, metrics } = require('@opentelemetry/api');
+const { context, propagation, trace, metrics, SpanStatusCode } = require('@opentelemetry/api');
 const cardValidator = require('simple-card-validator');
 const { v4: uuidv4 } = require('uuid');
 
@@ -24,65 +24,72 @@ function random(arr) {
 module.exports.charge = async request => {
   const span = tracer.startSpan('charge');
 
-  await OpenFeature.setProviderAndWait(flagProvider);
+  try {
+    await OpenFeature.setProviderAndWait(flagProvider);
 
-  const numberVariant =  await OpenFeature.getClient().getNumberValue("paymentFailure", 0);
+    const numberVariant = await OpenFeature.getClient().getNumberValue("paymentFailure", 0);
 
-  if (numberVariant > 0) {
-    // n% chance to fail with app.loyalty.level=gold
-    if (Math.random() < numberVariant) {
-      span.setAttributes({'app.loyalty.level': 'gold' });
-      span.end();
+    if (numberVariant > 0) {
+      // n% chance to fail with app.loyalty.level=gold
+      if (Math.random() < numberVariant) {
+        span.setAttributes({'app.loyalty.level': 'gold' });
 
-      throw new Error('Payment request failed. Invalid token. app.loyalty.level=gold');
+        throw new Error('Payment request failed. Invalid token. app.loyalty.level=gold');
+      }
     }
-  }
 
-  const {
-    creditCardNumber: number,
-    creditCardExpirationYear: year,
-    creditCardExpirationMonth: month
-  } = request.creditCard;
-  const currentMonth = new Date().getMonth() + 1;
-  const currentYear = new Date().getFullYear();
-  const lastFourDigits = number.substr(-4);
-  const transactionId = uuidv4();
-
-  const card = cardValidator(number);
-  const { card_type: cardType, valid } = card.getCardDetails();
-
-  const loyalty_level = random(LOYALTY_LEVEL);
-
-  span.setAttributes({
-    'app.payment.card_type': cardType,
-    'app.payment.card_valid': valid,
-    'app.loyalty.level': loyalty_level
-  });
-
-  if (!valid) {
-    throw new Error('Credit card info is invalid.');
-  }
+    const {
+      creditCardNumber: number,
+      creditCardExpirationYear: year,
+      creditCardExpirationMonth: month
+    } = request.creditCard;
+    const currentMonth = new Date().getMonth() + 1;
+    const currentYear = new Date().getFullYear();
+    const lastFourDigits = number.substr(-4);
+    const transactionId = uuidv4();
+
+    const card = cardValidator(number);
+    const { card_type: cardType, valid } = card.getCardDetails();
+
+    const loyalty_level = random(LOYALTY_LEVEL);
+
+    span.setAttributes({
+      'app.payment.card_type': cardType,
+      'app.payment.card_valid': valid,
+      'app.loyalty.level': loyalty_level
+    });
+
+    if (!valid) {
+      throw new Error('Credit card info is invalid.');
+    }
 
-  if (!['visa', 'mastercard'].includes(cardType)) {
-    throw new Error(`Sorry, we cannot process ${cardType} credit cards. Only VISA or MasterCard is accepted.`);
-  }
+    if (!['visa', 'mastercard'].includes(cardType)) {
+      throw new Error(`Sorry, we cannot process ${cardType} credit cards. Only VISA or MasterCard is accepted.`);
+    }
 
-  if ((currentYear * 12 + currentMonth) > (year * 12 + month)) {
-    throw new Error(`The credit card (ending ${lastFourDigits}) expired on ${month}/${year}.`);
-  }
+    if ((currentYear * 12 + currentMonth) > (year * 12 + month)) {
+      throw new Error(`The credit card (ending ${lastFourDigits}) expired on ${month}/${year}.`);
+    }
 
-  // Check baggage for synthetic_request=true, and add charged attribute accordingly
-  const baggage = propagation.getBaggage(context.active());
-  if (baggage && baggage.getEntry('synthetic_request') && baggage.getEntry('synthetic_request').value === 'true') {
-    span.setAttribute('app.payment.charged', false);
-  } else {
-    span.setAttribute('app.payment.charged', true);
-  }
+    // Check baggage for synthetic_request=true, and add charged attribute accordingly
+    const baggage = propagation.getBaggage(context.active());
+    if (baggage && baggage.getEntry('synthetic_request') && baggage.getEntry('synthetic_request').value === 'true') {
+      span.setAttribute('app.payment.charged', false);
+    } else {
+      span.setAttribute('app.payment.charged', true);
+    }
 
-  const { units, nanos, currencyCode } = request.amount;
-  logger.info({ transactionId, cardType, lastFourDigits, amount: { units, nanos, currencyCode }, loyalty_level }, 'Transaction complete.');
-  transactionsCounter.add(1, { 'app.payment.currency': currencyCode });
-  span.end();
+    const { units, nanos, currencyCode } = request.amount;
+    logger.info({ transactionId, cardType, lastFourDigits, amount: { units, nanos, currencyCode }, loyalty_level }, 'Transaction complete.');
+    transactionsCounter.add(1, { 'app.payment.currency': currencyCode });
 
-  return { transactionId };
+    return { transactionId };
+  } catch (err) {
+    span.recordException(err);
+    span.setStatus({ code: SpanStatusCode.ERROR, message: err.message });
+
+    throw err;
+  } finally {
+    span.end();
+  }
 };

src/payment/index.js

@@ -24,8 +24,7 @@ async function chargeServiceHandler(call, callback) {
   } catch (err) {
     logger.warn({ err })
 
-    span?.recordException(err)
-    span?.setStatus({ code: opentelemetry.SpanStatusCode.ERROR })
+    span?.setStatus({ code: opentelemetry.SpanStatusCode.ERROR, message: err.message })
     callback(err)
   }
 }

test/tracetesting/payment/amex-credit-card-not-allowed.yaml

@@ -35,3 +35,8 @@ spec:
     selector: span[tracetest.span.type="general" name="Tracetest trigger"]
     assertions:
     - attr:tracetest.response.status = 2
+  - name: It recorded an ERROR status on the charge span
+    selector: span[tracetest.span.type="general" name="charge"]
+    assertions:
+      - attr:tracetest.span.status_description = "Sorry, we cannot process amex credit cards. Only VISA or MasterCard is accepted."
+      - attr:tracetest.span.status_code = "STATUS_CODE_ERROR"

test/tracetesting/payment/expired-credit-card.yaml

@@ -35,3 +35,8 @@ spec:
     selector: span[tracetest.span.type="general" name="Tracetest trigger"]
     assertions:
     - attr:tracetest.response.status = 2
+  - name: It recorded an ERROR status on the charge span
+    selector: span[tracetest.span.type="general" name="charge"]
+    assertions:
+      - attr:tracetest.span.status_description = "The credit card (ending 0454) expired on 1/2021."
+      - attr:tracetest.span.status_code = "STATUS_CODE_ERROR"

test/tracetesting/payment/invalid-credit-card.yaml

@@ -35,3 +35,8 @@ spec:
     selector: span[tracetest.span.type="general" name="Tracetest trigger"]
     assertions:
     - attr:tracetest.response.status = 2
+  - name: It recorded an ERROR status on the charge span
+    selector: span[tracetest.span.type="general" name="charge"]
+    assertions:
+      - attr:tracetest.span.status_description = "Credit card info is invalid."
+      - attr:tracetest.span.status_code = "STATUS_CODE_ERROR"